Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rename validate_zimfile_creatable to validate_file_creatable #210

Merged
merged 3 commits into from
Nov 7, 2024

Conversation

elfkuzco
Copy link
Contributor

@elfkuzco elfkuzco commented Nov 4, 2024

Rationale

This PR renames the validate_zimfile_creatable method to validate_file_creatable to reflect its general applicability to check file creation beyond ZIM files. This resolves #200

Changes

  • Add validate_folder_creatable function to check if a folder can be written to.
  • Add validate_file_creatable function to check if a file with a given filename can be created in a given folder
  • Delegate validate_zimfile_creatable to use validate_file_creatable method with deprecation warning.
  • Add tests for new functions

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (ea6505f) to head (baa3e9d).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #210   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           38        38           
  Lines         2221      2223    +2     
  Branches       426       426           
=========================================
+ Hits          2221      2223    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@elfkuzco elfkuzco force-pushed the rename-validate-zimfile-creatable branch from 632d506 to 2b30a89 Compare November 4, 2024 14:21
@elfkuzco elfkuzco force-pushed the rename-validate-zimfile-creatable branch from 2b30a89 to ddbf524 Compare November 4, 2024 14:24
Copy link
Collaborator

@benoit74 benoit74 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I like the fact that we expose the method on folder, this is what I needed in another scraper indeed.

I think this is the typical example of something for which we do not want to maintain backward compatibility (we discussed about it with the team yesterday). Keeping old exception names is just weird / confusing.

I propose that we consider this as a real breaking change and:

  • rename all exceptions to remove any "ZIM" reference in their name (and clearly document these renaming in the changelog so that it is easier for scraperlib users to know about it)
  • completely drop the validate_zimfile_creatable method
  • bump version in __about__.py to 5.0.0-dev0 to indicate this will need a major (we already had doubts about whether other changes of the scraperlib deserved a major or not, this is somehow providing a good answer ^^)

@rgaudin OK for you?

@rgaudin
Copy link
Member

rgaudin commented Nov 5, 2024

Yes, fine with me.

I'd suggest we use a dedicated subsection in the changelog (### Breaking Changes instead of prefixing individual changes with BREAKING). I find is more practical

@benoit74
Copy link
Collaborator

benoit74 commented Nov 5, 2024

I'd suggest we use a dedicated subsection in the changelog (### Breaking Changes instead of prefixing individual changes with BREAKING). I find is more practical

Agreed. Let's add it to the bootstrap: openzim/_python-bootstrap#50

@elfkuzco
Copy link
Contributor Author

elfkuzco commented Nov 5, 2024

I propose that we consider this as a real breaking change and:

* rename all exceptions to remove any "ZIM" reference in their name (and clearly document these renaming in the changelog so that it is easier for scraperlib users to know about it)

* completely drop the `validate_zimfile_creatable` method

* bump version in `__about__.py` to `5.0.0-dev0` to indicate this will need a major (we already had doubts about whether other changes of the scraperlib deserved a major or not, this is somehow providing a good answer ^^)

@benoit74 , should I implement these on top of this PR?

Copy link
Collaborator

@benoit74 benoit74 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@benoit74 benoit74 merged commit 7b41b04 into openzim:main Nov 7, 2024
9 checks passed
@elfkuzco elfkuzco deleted the rename-validate-zimfile-creatable branch November 7, 2024 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename validate_zimfile_creatable to validate_file_creatable and add validate_folder_writable
3 participants