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 issue with missing required field default value in Navbar JSON schema. #130

Merged
merged 7 commits into from
Dec 28, 2023

Conversation

YiHuangDB
Copy link
Contributor

This is fix for #129 Demo code /docs Fetch error Internal Server Error /openapi.json

Issue:
when generating openapi schema for fastui component, for navibar component, default value for key = "required" is missing
Solution:
Add default value when generating schema

Note:
In docs some api point loading still very slow due to known issue: see #58

Copy link

codecov bot commented Dec 27, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (106d6c5) 94.01% compared to head (060e330) 94.01%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #130   +/-   ##
=======================================
  Coverage   94.01%   94.01%           
=======================================
  Files          11       11           
  Lines         718      718           
=======================================
  Hits          675      675           
  Misses         43       43           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@samuelcolvin
Copy link
Member

We should add a test.

Please have a go, if you can't I'll add one.

@YiHuangDB
Copy link
Contributor Author

YiHuangDB commented Dec 28, 2023

Added test for /docs endpoint, please review.

PS: I have trouble doing pre-commit check locally so I uninstall it, and then so many lint issue.... If I turn on pre-commit it report :

don't commit to branch...................................................Failed
- hook id: no-commit-to-branch

I need to comment out:

      #- id: no-commit-to-branch

And then:

[ERROR] Your pre-commit configuration is unstaged.
`git add .pre-commit-config.yaml` to fix this.

I don't know what to do then...

@samuelcolvin
Copy link
Member

@YiHuangIX contributing to open source (and software development in general) takes a fair git of persistence and willingness to solve problems yourself. You shouldn't just post every error you get to the project in the hope and expectation that someone else will tell you how to fix it. Your time is not more valuable than mine!

To solve pre-commit, you can:

  • add the -n flag to skip pre-commit completely
  • set the SKIP=no-commit-to-branch env variable when commiting
  • (the best answer, and something you should do anyway) not use your main branch creating pull requests

@samuelcolvin samuelcolvin changed the title Fix issue with missing required field default value in Navbar JSON schema. Fix for #129 Fix issue with missing required field default value in Navbar JSON schema. Dec 28, 2023
demo/tests.py Outdated


def test_doc():
r = client.get('/docs')
Copy link
Member

Choose a reason for hiding this comment

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

This test wouldn't pick up the error since /openapi.json hasn't been loaded - it would be loaded by the browser when getting /docs, but you're not using a browser here, just a test client.

In general it's worth reverting your fix and checking that your new test fails to confirm the test is doing its job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Saw your new test scripts. Well done!

@samuelcolvin samuelcolvin enabled auto-merge (squash) December 28, 2023 12:54
@samuelcolvin
Copy link
Member

thanks so much.

@samuelcolvin samuelcolvin merged commit d551f87 into pydantic:main Dec 28, 2023
14 checks passed
@YiHuangDB
Copy link
Contributor Author

@YiHuangIX contributing to open source (and software development in general) takes a fair git of persistence and willingness to solve problems yourself. You shouldn't just post every error you get to the project in the hope and expectation that someone else will tell you how to fix it. Your time is not more valuable than mine!

To solve pre-commit, you can:

  • add the -n flag to skip pre-commit completely
  • set the SKIP=no-commit-to-branch env variable when commiting
  • (the best answer, and something you should do anyway) not use your main branch creating pull requests

Thank you! For all your comments and feedbacks... I am still new to opensource communities, I am happy to work with leaders like you.

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.

2 participants