-
Notifications
You must be signed in to change notification settings - Fork 2
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
Vr/sqlmodel 20 #35
Vr/sqlmodel 20 #35
Conversation
…hema tests and stubs for integration with PSM
pipestat/argparser.py
Outdated
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.
Why were some constants moved here from const.py
?
@@ -35,23 +35,22 @@ def main(): | |||
status_schema_path=args.status_schema, | |||
flag_file_dir=args.flag_dir, | |||
) |
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.
namespace=args.namespace
status_schema_path=args.status_schema
Should be removed or they will throw an error during creation of psm.
@@ -35,23 +35,22 @@ def main(): | |||
status_schema_path=args.status_schema, | |||
flag_file_dir=args.flag_dir, | |||
) | |||
types_to_read_from_json = ["object"] + list(CANONICAL_TYPES.keys()) | |||
if args.command == REPORT_CMD: | |||
value = args.value | |||
if psm.schema is None: | |||
raise SchemaNotFoundError(msg="report", cli=True) | |||
result_metadata = psm.schema[args.result_identifier] |
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.
Now returning ParsedSchema instead of schema, should be:
result_metadata = psm.schema.results_data[args.result_identifier]
I removed entirely what was entirely unused (by the changes), and moved definitions for things that were used in just one file to that file.Here are some thought about cost/benefit tradeoff for this sort of thing…1. Using a constant variable rather than a hardcoded value is often nice, because it boosts maintainability by reducing the extent of what must change to change a value, and because it can bind a meaningful name to an otherwise mysterious value, giving it context/meaning. This, however, comes at the cognitive and somewhat mechanical cost of INDIRECTION: I have to look somewhere else to see the actual value of something.2. The indirection cost is elevated when I need to look in a different file to find the definition.3. If a constant is USED in only one file, there’s no upside to defining it in another file, but the indirection exacerbation is still there. Perhaps one could argue that knowing const.py exists and that hypothetical future use of the constant in another file would be simplified by being able to rely on “from const.py import *” rather than needing to know where the constant was defined to import it, but that’s often unlikely and feels like a premature optimization.5. If a constant is used in multiple places, the const.py idea starts to make more sense, but really only if there’s no “natural home” for the constant. Something like an atypical delimiter, for instance, may be perfect for this. Say you want to use “/“ rather than a more typical delimiter for parsing or writing text throughout your project. But even in the case of multi-file use of the constant, if there’s a “natural home,” or a place where something like the semantic/contextual essence of the constant is most clearly materialized or otherwise relevant, then that’s where it may make most sense to define it, as it communicates the is link to future code readers.So I tried to redistribute constant definitions according to these thoughts.Le 1 mai 2023 à 19:45, Donald C ***@***.***> a écrit :
@donaldcampbelljr commented on this pull request.
In pipestat/cli.py:
@@ -35,23 +35,22 @@ def main():
status_schema_path=args.status_schema,
flag_file_dir=args.flag_dir,
)
+ types_to_read_from_json = ["object"] + list(CANONICAL_TYPES.keys())
if args.command == REPORT_CMD:
value = args.value
if psm.schema is None:
raise SchemaNotFoundError(msg="report", cli=True)
result_metadata = psm.schema[args.result_identifier]
Now returning ParsedSchema instead of schema, should be:
result_metadata = psm.schema.results_data[args.result_identifier]
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
@CodiumAI-Agent /describe |
TitleVr/sqlmodel 20 TypeEnhancement, Tests DescriptionThis PR introduces significant changes and enhancements to the codebase, including:
Changes walkthrough
✨ Usage guide:Overview: When commenting, to edit configurations related to the describe tool (
With a configuration file, use the following template:
See the describe usage page for a comprehensive guide on using this tool. |
No description provided.