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.

Sign up for GitHub

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

Jump to bottom

[Tidy] Improve validation error message if CapturedCallable is directly provided #590

Merged
merged 14 commits into from
Jul 19, 2024
Merged

[Tidy] Improve validation error message if CapturedCallable is directly provided #590

merged 14 commits into from
Jul 19, 2024

Conversation

huong-li-nguyen
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen commented Jul 18, 2024 •
edited
Loading

Description

Thanks to @objectivitydrx for raising the bug and making a suggestion to improve our docs! 📝 🚀

Screenshot

Screenshot 2024-07-18 at 15 40 20

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
    • I have not referenced individuals, products or companies in any commits, directly or indirectly.
    • I have not added data or restricted code in any commits, directly or indirectly.

@huong-li-nguyen huong-li-nguyen changed the title Bug/validation error captured [Bug] Improve validation error message if CapturedCallable is directly provided Jul 18, 2024
@huong-li-nguyen huong-li-nguyen self-assigned this Jul 18, 2024
@huong-li-nguyen huong-li-nguyen linked an issue Jul 18, 2024 that may be closed by this pull request
kpi_card throws a nasty Pydantic Type missing error #588
Closed
1 task
@huong-li-nguyen huong-li-nguyen marked this pull request as ready for review July 18, 2024 14:36
@antonymilne
Refactor with same functionality
e17cff7 
* Change _DashboardReadyFigure to _SupportsCapturedCallable
* Make validator each_item=True and pre=False (default)
* Assign model to a new attribute to avoid mode-specific logic leaking into validator
@antonymilne
Refactor a little bit more
a5590df 
* Switch around logic to match other bits of code
* Rename field to value
@antonymilne
Fix message so it also works for Action
1876ff3
@antonymilne
Fix error message wording
525b850
@antonymilne antonymilne temporarily deployed to circleci_secrets July 18, 2024 20:30 — with GitHub Actions Inactive
antonymilne
antonymilne approved these changes Jul 18, 2024
View reviewed changes
Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

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

Amazing work, thanks for doing this so quickly! The tests are great and gave me a lot of confidence with my refactoring 👍

In theory we could extend the same check out to everywhere we use Action to prevent someone from inserting the action captured callable outside the Action model. But that's less likely to happen and also much more widespread throughout our code so would be more effort to implement. So I think don't bother doing it unless you think we should.

@antonymilne antonymilne temporarily deployed to circleci_secrets July 18, 2024 20:38 — with GitHub Actions Inactive
maxschulz-COL
maxschulz-COL approved these changes Jul 19, 2024
View reviewed changes
Copy link
Contributor

@maxschulz-COL maxschulz-COL left a comment

Choose a reason for hiding this comment

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

⭐ Really nice work you two ⭐

Basically just one question: what was the bug here (rather than the usability and confusing message) - maybe I missed something 🙃

vizro-core/changelog.d/20240718_151233_huong_li_nguyen_validation_error_captured.md Show resolved Hide resolved
vizro-core/src/vizro/models/_components/_form.py Show resolved Hide resolved
vizro-core/src/vizro/models/_page.py Outdated Show resolved Hide resolved
@antonymilne
Copy link
Contributor

⭐ Really nice work you two ⭐

Basically just one question: what was the bug here (rather than the usability and confusing message) - maybe I missed something 🙃

Yeah, you're right - there wasn't any bug here, just an improved error message now. So I agree this would go in the changelog better as Changed or maybe even Added (like Added validation for blah blah).

@huong-li-nguyen huong-li-nguyen changed the title [Bug] Improve validation error message if CapturedCallable is directly provided [Tidy] Improve validation error message if CapturedCallable is directly provided Jul 19, 2024
@huong-li-nguyen huong-li-nguyen enabled auto-merge (squash) July 19, 2024 09:06
@huong-li-nguyen
Copy link
Contributor Author

So I think don't bother doing it unless you think we should.

Agree, I don't think we need to do it for now 👍 Thanks for the commits! I forgot about the each_item argument for the pydantic validators. Nice clean up 👍

@petar-qb petar-qb removed their request for review July 19, 2024 09:08
@huong-li-nguyen huong-li-nguyen merged commit bf65329 into main Jul 19, 2024
30 checks passed
@huong-li-nguyen huong-li-nguyen deleted the bug/validation-error-captured branch July 19, 2024 09:17
nadijagraca pushed a commit that referenced this pull request Aug 1, 2024
@huong-li-nguyen @antonymilne @nadijagraca
[Tidy] Improve validation error message if CapturedCallable is dire…
77279a8 
…ctly provided (#590)

Co-authored-by: Antony Milne <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antonymilne antonymilne antonymilne approved these changes

@maxschulz-COL maxschulz-COL maxschulz-COL approved these changes

@Joseph-Perkins Joseph-Perkins Awaiting requested review from Joseph-Perkins Joseph-Perkins is a code owner

@lingyielia lingyielia Awaiting requested review from lingyielia lingyielia is a code owner

@l0uden l0uden Awaiting requested review from l0uden

@nadijagraca nadijagraca Awaiting requested review from nadijagraca

Assignees

@huong-li-nguyen huong-li-nguyen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kpi_card throws a nasty Pydantic Type missing error
3 participants
@huong-li-nguyen @antonymilne @maxschulz-COL