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

add check for root_type specified for json schema generation #5622

Merged
merged 1 commit into from
Nov 11, 2019
Merged

add check for root_type specified for json schema generation #5622

merged 1 commit into from
Nov 11, 2019

Conversation

LizardWizzard
Copy link
Contributor

Hi! As recommended here I've added simple check to avoid segfault. In my opinion this solution currently misses user friendly error message, but I dont see any example near this place in codebase for propagating error message. Can you advise me how to properly do this? Also I'll be glad to add test for change I made, so the question is what is the best place for such test? Can I simply add fbs file without root_type and test return code of flatc in generate_code.sh or something else?

Also recommendations for PR says:

For any C++ changes, please make sure to run sh src/clang-format-git.sh

Firstly script is renamed to just clang-format.sh and after I launched it there are format related changes to code that is not related to my changes. So definetly this check should be in CI to protect the health of the codebase.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@LizardWizzard
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@aardappel
Copy link
Collaborator

You don't have the right clang-format script because you haven't rebased to latest master, but that's ok in this case.

Thanks!

@aardappel aardappel merged commit cbbd6ac into google:master Nov 11, 2019
LuckyRu pushed a commit to LuckyRu/flatbuffers that referenced this pull request Oct 2, 2020
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.

3 participants