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.
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
Update bundle related methods #99
base: main
Are you sure you want to change the base?
Update bundle related methods #99
Changes from all commits
b654f0e
ca57f22
d53f1aa
3167964
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this is here to create a type like
Dict[str,Any]
in quite a few places to indicate a a dict for templating contextMaybe its worth defining a local type like
to use when we intend to render some context
technically jinja2 is very loose on it's typing.
effectively...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this changes the signature of this method. We'll need to major rev the package and update the docs accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not actually convinced that the signature actually needs to change. The method signature and docstrings of an API essentially define the contract for method itself. The existing contract is fairly vague in the specification, but clear in the parameters. This change can largely (if not wholly) still honor the contract of the method without making a breaking API change. An additional advantage/improvement would be to further refine and clarify the method contract in the docstrings.
As there are existing users of this library, if the API is changed then it will break a downstream consumer of the library. Since the method contract can be upheld with the existing signature, it seems better to me to not change the signature and subsequently not break downstream consumers.
Just my $0.02
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method now has a new signature and will also need the documentation updated