notionapi icon indicating copy to clipboard operation
notionapi copied to clipboard

Add AudioBlock implementation with tests

Open aydinomer00 opened this issue 1 year ago • 7 comments

Fixes #183

This PR adds AudioBlock implementation with the following changes:

  • Added BlockTypeAudio to block types
  • Implemented DownloadableFileBlock interface for AudioBlock
    • Implemented GetURL() method for retrieving internal/external audio URLs
    • Implemented GetExpiryTime() method for handling file expiry
  • Added comprehensive test coverage
    • Tests for internal and external URLs
    • Tests for expiry time functionality
    • Interface compliance checks

All tests are passing. The implementation follows the same pattern as other media blocks (Image, Video) in the codebase.

aydinomer00 avatar Dec 20 '24 22:12 aydinomer00

Thanks for the feedback! I'll move the BlockTypeAudio constant to be grouped with other media types, placing it between image and video blocks for better organization.

aydinomer00 avatar Dec 23 '24 10:12 aydinomer00

Thanks for the feedback! I've moved the BlockTypeAudio constant to be grouped with other media types, placing it between image and video blocks for better organization.

Eugene @.***>, 21 Ara 2024 Cmt, 18:27 tarihinde şunu yazdı:

@.**** commented on this pull request.

In const.go https://github.com/jomei/notionapi/pull/187#discussion_r1894646135:

@@ -218,6 +218,8 @@ const ( BlockTypeChildPage BlockType = "child_page" BlockTypeChildDatabase BlockType = "child_database"

  • BlockTypeAudio BlockType = "audio"

Other constants are grouped. Maybe then put this BlockTypeAudo after image and before video so it's grouped as well?

— Reply to this email directly, view it on GitHub https://github.com/jomei/notionapi/pull/187#pullrequestreview-2518678191, or unsubscribe https://github.com/notifications/unsubscribe-auth/A2AW4K5CYQWTMHRAD3NZVSL2GWCHFAVCNFSM6AAAAABT75UCPWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMJYGY3TQMJZGE . You are receiving this because you authored the thread.Message ID: @.***>

aydinomer00 avatar Dec 23 '24 10:12 aydinomer00

@aydinomer00 sync please your branch, so it doesn't have conflicts for merging. I guess then it will be sooner to be merged by maintainer.

P.S. I'm ok sitting with local branch-on-top-of-branches, but i'm looking forward to getting this merged as well :)

amberpixels avatar Dec 23 '24 11:12 amberpixels

@amberpixels Thank you so much for your patience and guidance! I’m still learning the ropes with Git and doing my best to follow your instructions. I’ve tried to sync my branch and resolve the conflicts, but I’m not entirely sure if everything was done correctly. If you notice any mistakes or have additional suggestions, I’d really appreciate your feedback. Thanks again for all your help!

aydinomer00 avatar Jan 01 '25 16:01 aydinomer00

hey @aydinomer00. There are several ways of resolving the issue. I don't claim my approach is the most comfortable, but here what you can do:

  1. Your fork aydinomer00/notionapi is 1 commit behind from jomei/notionapi:main. You'll see the button Sync there on the main page of your fork.
  2. after your main branch is up-to-date you can either merge or rebase. In this case i personally prefer merging. So you checkout your feature branch and do git merge main. After resolving conflicts and commiting you can push to your branch again.

amberpixels avatar Jan 01 '25 22:01 amberpixels

@amberpixels I just used the “Update branch” button to sync my branch with the upstream repository. It looks like there aren’t any conflicts so far. Could you please take a look and let me know if you spot any issues or have any suggestions? Thanks!

aydinomer00 avatar Jan 02 '25 07:01 aydinomer00

@aydinomer00 could you please resolve conflicts in this PR? I can easily merge it then

jomei avatar Feb 19 '25 06:02 jomei