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
feat: add
output_type
parameter (#519) #562feat: add
output_type
parameter (#519) #562Changes from 5 commits
aad466d
0191615
0eca724
cb0e433
6cafc12
8ae345f
6f7d14c
14d24ea
71e1fd4
0ccaf2d
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'd rather use df_config.df_type like this:
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 also allows you not having to import polars and pandas!
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.
Excellent, since this releases the module from having excessive imports of third party.
Done
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.
The
PlotOutputType
class uses a regular expression to validate that the actual value is a string representing a valid file path. This is a good approach, but it might be worth considering whether there are any edge cases that this regex doesn't cover. For example, it doesn't seem to handle spaces in file paths.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 would pass it as part of default values in the
generate python code
prompt only, so that this is accessible both on the default prompt and in the custom prompts. This might not be needed in every prompt in the future.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.
Sure, you're right, it makes the additional context to be in one place
default_values
.Apologize for spreading the setting logic like this, just we've got rly complicated implementation in
._get_prompt()
as for me. As i've mentioned: some variables're being set in__init__()
, the rest are being set in_get_prompt()
itself, when iterating overdefault_values
. That was kind a tricky moment when i'd had output types mapping inside prompt class before.Done
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.
@nautics889 yeah, totally, it's super involved, we'll definitely need to simplify that a little bit in the future 😄
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.
The
output_type_hint
is now passed to the_get_prompt()
method. This change assumes that all prompts will accept this argument. If there are any prompts that do not accept this argument, this will cause an error.