Warn on unused arguments #238
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adds a new flag, warn_unused, to Template.evaluate; defaults to True. This raises a warning if not all of the mandatory parameters are provided to the Template when it is evaluated. Will raise warning if optional arguments are required (when using the corresponding flag on Template.evaluate)
|
There's a subtle logic bug in here that is causing the tests to fail; I'm anticipating a relatively minor change to the code, but feel free to hold off on review for a day |
TShapinsky
reviewed
May 8, 2023
Co-authored-by: Tobias Shapinsky <[email protected]>
TShapinsky
reviewed
May 8, 2023
They won't be, I don't think. There's a subtle issue with dependencies more than 1 'hop' away that I'm still trying to fix |
Due to how we have the dependencies structured, the mapsTo relationship needs to refer to a connection point that lies in a dependency's dependency. This makes that relationship explicit
New problem is when we are testing the libraries for validity, sometimes we want to give an explicit argument to the 'mapsto' optional arg w/n a dependency. However, we currently can't give a binding to a nested dependency. The result is that we end up with multiple mapsto (because the 'optional' mapsto argument gets a newly minted URI from template.fill()), which fails validation. This commit splits template dependency management into 2 phases: an initial phase which adds the dependency links and args to the DB; a second phase which checks that the args/params between the template and its dependencies are all valid and work as expected.
…OTIF into gtf-warn-unused-issue-203
|
Logic bug is fixed as discussed on the call today -- this is ready for review! |
MatthewSteen
requested changes
May 11, 2023
Co-authored-by: Matt Steen <[email protected]>
Co-authored-by: Matt Steen <[email protected]>
Co-authored-by: Matt Steen <[email protected]>
Co-authored-by: Matt Steen <[email protected]>
Co-authored-by: Matt Steen <[email protected]>
Co-authored-by: Matt Steen <[email protected]>
Co-authored-by: Matt Steen <[email protected]>
TShapinsky
reviewed
May 11, 2023
MatthewSteen
approved these changes
May 12, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #203 and #237
Cleans up template evaluation with some more utility functions (all tested), and some additional comments. Also adds a few tests to cover untested edge cases in template evaluation. Fixes behavior around inlining dependencies with optional arguments (#237)