Improve ConfigData & related #101
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
* finalize ConfigWorkflow canonicalization * improve type hints
leclairm
reviewed
Jan 21, 2025
agoscinski
reviewed
Jan 22, 2025
| case _: | ||
| msg = f"{cls.__name__} requires name to be a str (got {key})." | ||
| raise TypeError(msg) | ||
| return data |
There was a problem hiding this comment.
Does this make sense to be a classmethod? I typically see classmethods used only for creating an instance the class (so this function would dict -> _NamedBaseModel). Here we create a dict object. Don't you think a staticmethod is more transparently communicating that not an instance of the same class is created?
There was a problem hiding this comment.
My reasons:
- Model validators in pydantic docs are classmethods (this is only a separate method to allow additional usage).
- It allows access to
cls.__name__, which will report the name of the subclass. This is more helpful and less confusing than always reporting the name of the base class. - This class method is not designed to deal with arbitrary dicts. Semantically, it is not intended to have meaning outside this class. Therefore it violates the rule "staticmethod could also be standalone function"
- If there is value in it being designed for standalone usage, I would prefer to make it a standalone function and design and test it accordingly.
There was a problem hiding this comment.
I actually changed the usage in get_plugin_from_named_base_model to use the ConfigBaseTask.extract_merge_name, which should cause errors to report the right class name there as well.
agoscinski
approved these changes
Jan 23, 2025
leclairm
approved these changes
Jan 23, 2025
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.
_NamedBaseModelto allow alternative creation of models with intuitive kwargs from python.typeand.srcnon-optional inConfigBaseDataSpecsConfigBaseDataSpecse.typetype to an enum (replaces string-based validator while improving legibility)