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

Refactor parse_access_* functions into BaseBuilder class #181

Merged
merged 11 commits into from
Aug 19, 2024

Conversation

marc-white
Copy link
Collaborator

Fixes #67 (or at least improves).

This PR refactors the utils functions parse_access_filename and parse_access_ncfile to be @classmethods of the builders.BaseBuilder class. The idea behind doing this is:

  • The functions remain available to external callers as, e.g., BaseBuilder.parse_access_filename;
  • Having them as @classmethod (as opposed to @staticmethod) gives access to the underlying class definition, so every Builder now has a list of relevant patterns (PATTERNS) that is used by default for this builder. This has the effect of causing each Builder to have its own unique list of filename regex patterns, preventing cross-contamination when new builders/patterns are added. The patterns list used can be defined by kwarg to the functions if required instead;
  • Specifying a custom version of parse_access_* for a particular Builder is now easier (although I'm not sure if that's something that will ever need to be done).

Associated tests have also been updated.

@marc-white marc-white linked an issue Aug 2, 2024 that may be closed by this pull request
Copy link

codecov bot commented Aug 2, 2024

Codecov Report

Attention: Patch coverage is 98.50746% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.96%. Comparing base (a808275) to head (8465fb7).

Files Patch % Lines
src/access_nri_intake/source/builders.py 98.50% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #181   +/-   ##
=======================================
  Coverage   96.96%   96.96%           
=======================================
  Files           9        9           
  Lines         626      626           
=======================================
  Hits          607      607           
  Misses         19       19           

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

Copy link
Collaborator

@dougiesquire dougiesquire left a comment

Choose a reason for hiding this comment

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

Thanks for this @marc-white!

I guess the main difference in having these as @classmethods, rather than just having source.utils.parse_access_ncfile receive an additional input for the patterns (which are an attribute on each Builder class) is that it allows overriding the parse_access_ncfile and/or parse_access_filename. I'm not sure whether that will ever be needed, but I guess there's no reason not to add that additional flexibility.

@marc-white
Copy link
Collaborator Author

@dougiesquire is there any other process we need to tick off before I merge this back in, or is the accepted review enough?

@dougiesquire
Copy link
Collaborator

An accepted review and the passing checks is sufficient. Please feel free to merge

@marc-white marc-white merged commit a07470a into main Aug 19, 2024
17 checks passed
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.

Need a better way to get dataset keys
2 participants