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

Resolves ENGA-923 "Modify django-bigquery-exporter base to include decorator for custom_field" #6

Merged
merged 7 commits into from
Nov 29, 2023

Conversation

DevDaveFrame
Copy link
Collaborator

@DevDaveFrame DevDaveFrame commented Nov 28, 2023

ENGA-923

Non-technical Context

Replacing the custom_fields array with a single fields array and a decorator to identify custom methods allows for a cleaner implementation and higher visibility for custom fields

Technical Context

The current decorator implementation is very bare bones. It just adds a custom attribute, "is_custom_field" to the function or method. This allows the _process_queryset method to check for the presence of the field, and determine whether to check the model or call the method.

The decorator isn't strictly necessary in this implementation but it does reduce the risk of accidental naming collisions with existing model fields and it opens up the possibility of additional transformations or validation in the future. And it makes it easier to see what methods are custom fields at a glance.

While writing tests for the new decorator, I also refactored the existing tests to DRY up the test class by using a fixture rather than rewriting the test exporter class every time.

Developer

  • Ensure code complies with the style guide
  • Devise edge cases to test the bug fix or feature being merged
  • Make sure the code is clean and understandable
  • Ensure code is not overly inefficient
  • Code changes include a migration
    • Migration modifies large table and has been run against full db to estimate completion time
    • Migration modifies a small table and has only been run against slim db
    • There are no conflicting migration leaf nodes
  • Ensure documentation is understandable and accurate, including:
    • Code comments and doc strings
    • Technical documentation

Reviewer

  • Ensure code complies with the style guide
    • If there are few or minor issues, add inline comments
    • If there are many issues or it seems like the developer did not follow the style guide, leave a comment asking them to do so
  • Devise edge cases to test the bug fix or feature being merged
  • Make sure the code is clean and understandable
    • Ask for inline comments on unclear sections
  • Ensure code is not overly inefficient
  • Ensure documentation is understandable and accurate, including:
    • Code comments and doc strings
    • Technical documentation
  • There are no conflicting migration leaf nodes

@echan217 echan217 self-requested a review November 29, 2023 17:03
echan217
echan217 previously approved these changes Nov 29, 2023
Copy link
Contributor

@echan217 echan217 left a comment

Choose a reason for hiding this comment

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

LGTM! Nice refactor!

@DevDaveFrame DevDaveFrame merged commit 6f4cc9c into main Nov 29, 2023
1 check passed
@DevDaveFrame DevDaveFrame deleted the ENGA-933-custom-field-decorator branch November 29, 2023 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants