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

[TS] Allows object API to set 0 for a null-default scalar. #7864

Merged
merged 13 commits into from
Sep 12, 2023
Merged

[TS] Allows object API to set 0 for a null-default scalar. #7864

merged 13 commits into from
Sep 12, 2023

Conversation

jviel-beta
Copy link
Contributor

Fixes #7863

@google-cla
Copy link

google-cla bot commented Mar 10, 2023

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

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@github-actions github-actions bot added c++ codegen Involving generating code from schema javascript typescript labels Mar 10, 2023
@jviel-beta
Copy link
Contributor Author

I did not include any new test cases.
I haven't looked into your CI, but I believe there's a problem with pathing and dependencies for the TypeScriptTest.py.
TestAll.sh cannot find the script, and the script does not check if esbuild is installed

@dbaileychess
Copy link
Collaborator

Is there any generated code that have these changes? You might need to run scripts/generate_code.py to apply them and include them in the PR.

The TypeScriptTest.py seems right: https://github.com/google/flatbuffers/blob/master/tests/ts/TypeScriptTest.py#L25-L28

The TestAll.sh seems broken: https://github.com/google/flatbuffers/blob/master/tests/TestAll.sh#L19 that should have the right path. Not sure if people use this often, but feel free to update in this PR.

tungmeoo
tungmeoo reviewed Mar 15, 2023
View reviewed changes
Copy link

@tungmeoo tungmeoo left a comment

Choose a reason for hiding this comment

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

ok

@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2023

This pull request is stale because it has been open 3 weeks with no activity. Please comment or label not-stale, or this will be closed in 7 days.

@github-actions github-actions bot added the stale label Apr 5, 2023
@jviel-beta
Copy link
Contributor Author

not stale

@github-actions github-actions bot removed the stale label Apr 7, 2023
dbaileychess
dbaileychess previously approved these changes Apr 26, 2023
View reviewed changes
@dbaileychess dbaileychess enabled auto-merge (squash) April 28, 2023 18:22
@dbaileychess
Copy link
Collaborator

@jviel-beta looks like you need to run the scripts/generate_code.py script and check in the results.

auto-merge was automatically disabled April 28, 2023 18:55

Head branch was pushed to by a user without write access

@jviel-beta
Copy link
Contributor Author

@jviel-beta looks like you need to run the scripts/generate_code.py script and check in the results.

done!
Also updated the path for TypeScriptTests.py in TestAll.sh

@github-actions
Copy link
Contributor

This pull request is stale because it has been open 3 weeks with no activity. Please comment or label not-stale, or this will be closed in 7 days.

@github-actions github-actions bot added the stale label May 19, 2023
@jviel-beta
Copy link
Contributor Author

not-stale

@github-actions github-actions bot removed the stale label May 22, 2023
@jviel-beta
Copy link
Contributor Author

jviel-beta commented Aug 3, 2023 •
edited
Loading

@dbaileychess or @tungmeoo Can I please get a review on this? It's been sitting for a while now and we'd like to remove the fork. Thanks

@jviel-beta
Copy link
Contributor Author

@dbaileychess @aardappel. Is anyone available to approve the workflows?

@github-actions github-actions bot added the nim label Sep 11, 2023
@jviel-beta
Copy link
Contributor Author

@aardappel Thank you. Looks like the failing tests are not related to my changes... just a clang unused symbol error. Pushed a potential fix.

@jviel-beta
Copy link
Contributor Author

@aardappel, sorry but I think I need help with the nim failures. Not sure why my PR would affect the nim tests in any way

@bjornharrtell bjornharrtell requested review from bjornharrtell and removed request for tungmeoo September 11, 2023 19:54
bjornharrtell
bjornharrtell requested changes Sep 11, 2023
View reviewed changes
Copy link
Collaborator

@bjornharrtell bjornharrtell left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This LGTM except for the comment on the nim change, @jviel-beta if you can adress that I think we can merge this (regardless of nim test failure, I think it fails due to completely unrelated reasons and we can force the merge).

@github-actions github-actions bot removed the nim label Sep 12, 2023
@bjornharrtell bjornharrtell merged commit f625ff3 into google:master Sep 12, 2023
@jmillan jmillan mentioned this pull request Feb 29, 2024
Closed
This was referenced Mar 1, 2024
Closed
Merged
@BrewTestBot BrewTestBot mentioned this pull request Mar 8, 2024
Merged
jochenparm pushed a commit to jochenparm/flatbuffers that referenced this pull request Oct 29, 2024
@jviel-beta @dbaileychess
[TS] Allows object API to set 0 for a null-default scalar. (google#7864)
18ea40c 
* Fixes bug where null default allows 0 as a value.

* Undoes one bit, adds null type allowance to addField<> default.

* Undoes IDE auto-format of imports.

* Adds generated changes after scripts/generate_code.py

* Removes unused symbol.

* Revert "Removes unused symbol."

This reverts commit 9cece17.

---------

Co-authored-by: Derek Bailey <[email protected]>
jochenparm pushed a commit to jochenparm/flatbuffers that referenced this pull request Oct 29, 2024
@jviel-beta @dbaileychess
[TS] Allows object API to set 0 for a null-default scalar. (google#7864)
da2cfc0 
* Fixes bug where null default allows 0 as a value.

* Undoes one bit, adds null type allowance to addField<> default.

* Undoes IDE auto-format of imports.

* Adds generated changes after scripts/generate_code.py

* Removes unused symbol.

* Revert "Removes unused symbol."

This reverts commit 9cece17.

---------

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

@tungmeoo tungmeoo tungmeoo left review comments

@bjornharrtell bjornharrtell bjornharrtell approved these changes

@dbaileychess dbaileychess Awaiting requested review from dbaileychess

Assignees
No one assigned
Labels
c++ codegen Involving generating code from schema javascript typescript
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[TS] Object API cannot send a 0 for null default scalar without forceDefaults.
4 participants
@jviel-beta @dbaileychess @bjornharrtell @tungmeoo