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.

Sign up for GitHub

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

Jump to bottom

refactor: ♻️ use checks in write_resource_properties() #990

Merged
merged 97 commits into from
Jan 27, 2025
Merged

refactor: ♻️ use checks in write_resource_properties() #990

merged 97 commits into from
Jan 27, 2025

Conversation

martonvago
Copy link
Contributor

@martonvago martonvago commented Jan 22, 2025 •
edited by lwjohnst86
Loading

Description

This PR switches to the new check functions in write_resource_properties.

There is probably an argument for reorganising this function a bit, but in this PR I am just adding the check functions.

This PR needs an in-depth review.

Checklist

  • Added or updated tests
  • Updated documentation
  • Ran just run-all

martonvago and others added 30 commits December 19, 2024 15:03
@martonvago
refactor: ♻️ use custom CheckError in checks
55c03c6
@martonvago
feat: ✨ improve CheckError
958bb18
@martonvago
test: ✅ add tests for helper functions
a619ded
@martonvago
refactor: ♻️ return early from function
3586c51
@martonvago
docs: 📝 update test docstrings
d3de146
@martonvago @lwjohnst86
apply suggestions from code review
05e7d41 
Co-authored-by: Luke W. Johnston <[email protected]>
@pre-commit-ci
chore(pre-commit): ✏️ automatic fixes
52d3a24
@martonvago
Merge branch 'main' into feat/check-error
64d7a87
@martonvago
feat: ✨ put required fields into constants
8196dc0
@martonvago
feat: ✨ add simple helper functions
8c701d8
@martonvago
feat: ✨ add functions to check required and blank fields
b6a5a86
@martonvago
feat: ✨ add functions for collecting Sprout-specific package and reso…
e9da123 
…urce errors
@martonvago
feat: ✨ add check_resource_properties
6133ac9
@martonvago
feat: ✨ add check_package_properties
b179dbe
@martonvago
feat: ✨ add check_properties
166958b
@martonvago
docs: 📝 fix docstring
dabbb75
@martonvago @signekb
apply suggestions from code review
a08ce8b 
Co-authored-by: Signe Kirk Brødbæk <[email protected]>
@martonvago
refactor: ♻️ rename function to validation_errors_to_check_errors
1dd1e1e
@martonvago
refactor: ♻️ rename file to validation_errors_to_check_errors
c595aa9
@martonvago
docs: 📝 add more detail to docstring
cb49bd1
@martonvago
refactor: ♻️ rename constant to PACKAGE_RECOMMENDED_FIELDS
d603d82
@martonvago
fix: 🐛 include data in resource required fields
abfc4c5
@martonvago
refactor: ♻️ make structure of PACKAGE_SPROUT_REQUIRED_FIELDS consistent
6611019
@martonvago
refactor: ♻️ drop fields using pop
fea4a5f
@martonvago @signekb
apply suggestions from code review
1cbec8a 
Co-authored-by: Signe Kirk Brødbæk <[email protected]>
@martonvago
refactor: ♻️ rename function to get_sprout_specific_resource_errors
c5b784f
@martonvago
refactor: ♻️ rename file to get_sprout_specific_resource_errors
c854a2d
@martonvago
docs: 📝 update test names and docstrings
ee5edf0
@martonvago
Merge branch 'feat/check-error' into feat/sprout-checks-1-required-fi…
ee05805 
…elds
@martonvago
Merge branch 'feat/sprout-checks-1-required-fields' into feat/sprout-…
e43d13e 
…checks-2-simple-helper-functions
martonvago and others added 15 commits January 20, 2025 16:44
@martonvago
style: 🎨 fix linter
62ab91f
@martonvago
test: ✅ don't check error message in test
b8c7d1c
@martonvago
Merge branch 'feat/sprout-checks-5-check-resource-properties' into fe…
54e3a6c 
…at/sprout-checks-6-check-package-properties
@martonvago @lwjohnst86
apply suggestions from code review
cdbc133 
Co-authored-by: Luke W. Johnston <[email protected]>
@martonvago
style: 🎨 fix linter
845d9f2
@martonvago
Merge branch 'feat/sprout-checks-6-check-package-properties' into fea…
2cfd170 
…t/sprout-checks-7-check-properties
@martonvago @lwjohnst86
apply suggestions from code review
e06afa4 
Co-authored-by: Luke W. Johnston <[email protected]>
@martonvago
Merge branch 'feat/sprout-checks-7-check-properties' into feat/add-ch…
a35915b 
…eck-error-matcher
@martonvago
Merge branch 'feat/add-check-error-matcher' into refactor/use-check-e…
1281477 
…rror-matcher
@martonvago
Merge branch 'main' into feat/add-check-error-matcher
6ec1bdb
@martonvago
Merge branch 'feat/add-check-error-matcher' into refactor/use-check-e…
06ec745 
…rror-matcher
@lwjohnst86
Merge branch 'main' into feat/add-check-error-matcher
ea10182
@martonvago
Merge branch 'main' into feat/add-check-error-matcher
05a3bf5
@martonvago
Merge branch 'feat/add-check-error-matcher' into refactor/use-check-e…
25f74d7 
…rror-matcher
@martonvago
refactor: ♻️ use check functions in write_resource_properties
2cfb4cc
@martonvago martonvago self-assigned this Jan 22, 2025
martonvago
martonvago commented Jan 22, 2025
View reviewed changes
tests/core/test_write_resource_properties.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could do with a couple more tests, but I expect the function to change a bit, so I'll add them a bit later.

@martonvago
Copy link
Contributor Author

Having looked at these functions a bit, maybe a natural organisation would be:

  • edit_package_properties (leave as is):
    • takes a partial PackageProperties
    • returns a complete PackageProperties, ready to write to file
  • edit_resource_properties (= most of the current write_resource_properties):
    • takes a partial ResourceProperties
    • returns a complete PackageProperties, ready to write to file
  • write_properties (= the current write_package_properties)
    • takes a complete PackageProperties

The big change here would be that edit_resource_properties would return a PackageProperties (with the ResourceProperties updated or added). This gives a nice flow, but I'm not sure if it's misleading?

@martonvago martonvago marked this pull request as ready for review January 22, 2025 15:33
@martonvago martonvago requested a review from a team as a code owner January 22, 2025 15:33
@lwjohnst86 lwjohnst86 changed the title refactor: ♻️ use check functions in write_resource_properties refactor: ♻️ use checks in write_resource_properties() Jan 23, 2025
Base automatically changed from refactor/use-check-error-matcher to main January 23, 2025 10:10
@martonvago martonvago mentioned this pull request Jan 23, 2025
Merged
3 tasks
@lwjohnst86 lwjohnst86 mentioned this pull request Jan 23, 2025
Open
lwjohnst86
lwjohnst86 approved these changes Jan 27, 2025
View reviewed changes
Copy link
Member

@lwjohnst86 lwjohnst86 left a comment

Choose a reason for hiding this comment

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

👍

@lwjohnst86 lwjohnst86 merged commit c10d053 into main Jan 27, 2025
3 checks passed
@lwjohnst86 lwjohnst86 deleted the refactor/use-checks-in-write-res-props branch January 27, 2025 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lwjohnst86 lwjohnst86 lwjohnst86 approved these changes

Assignees

@martonvago martonvago

Labels
None yet
Projects
Team project planning
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@martonvago @lwjohnst86