Skip to content
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

Fix: Empty string as template in CustomPlot.init #841

Merged
merged 5 commits into from
Sep 5, 2024

Conversation

anunayasri
Copy link
Contributor

@anunayasri anunayasri commented Sep 4, 2024

Fixes #10482 (report in dvc repo) at iterative/dvc#10482

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

@dberenbaum
Copy link
Collaborator

Thanks @anunayasri! As long as we are fixing it for the empty string, can we also handle any string that's not a supported template?

@shcheklein
Copy link
Member

Thanks @anunayasri! As long as we are fixing it for the empty string, can we also handle any string that's not a supported template?

@dberenbaum how do we recognize that? run Vega parser / validator?

Comment on lines 26 to 27
if template is not None and template.strip() == "":
template = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just disallow empty string.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, this template.strip() is unnecessary.

Suggested change
if template is not None and template.strip() == "":
template = None
if not template:
template = None

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skshetry Thanks. This is much cleaner.

Co-authored-by: skshetry <18718008+skshetry@users.noreply.github.com>
@anunayasri
Copy link
Contributor Author

There is a missing None check in my commit and mypy failing the pipeline. I have correct it.
I observed that $ pre-commit run --all-files does not run mypy in the local. Is it by design? May be to speed up the pre-commit exeuction.

}


def test_default_template_in_log_custom_plot(tmp_dir):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's worth it to add this test, has very less return.

We can't test for every combinations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since template = '' caused a cryptic error, I added this test as a guardrail to ensure that any template = None is handled explicitly.
But on a second thought, I think you are correct. This test is for log_plot() which has default value for template. CustomPlot.__init__() doesn't have a default value for template but that shouldn't be test here.
Will rectify.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 95.18%. Comparing base (602c053) to head (bf8ff74).
Report is 32 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #841      +/-   ##
==========================================
- Coverage   95.47%   95.18%   -0.29%     
==========================================
  Files          57       55       -2     
  Lines        3889     3904      +15     
  Branches      353      349       -4     
==========================================
+ Hits         3713     3716       +3     
- Misses        124      138      +14     
+ Partials       52       50       -2     

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@dberenbaum
Copy link
Collaborator

@dberenbaum how do we recognize that? run Vega parser / validator?

πŸ€” I forgot we don't render these anywhere here, and supporting custom templates would not be easy, so forget my initial suggestion.

@shcheklein shcheklein merged commit 6888462 into iterative:main Sep 5, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants