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

[Nim] Bfbs Nim Generator #7534

Merged
merged 41 commits into from Oct 21, 2022
Merged

[Nim] Bfbs Nim Generator #7534

merged 41 commits into from Oct 21, 2022

Conversation

danlapid
Copy link
Contributor

Based on the existing languages (mostly - swift, python, lua) and on https://github.com/RecruitMain707/NimFlatbuffers
Relevant issue: #5855
Replaces: #7362

So I had more time to work on it and seeing as how @dbaileychess requested I use the bfbs format instead I though it'd be wiser to just start with a clean branch and add only what's relevant.
Also, in accordance with @CasperN 's suggestions I submit this PR without OneFile nor Object Api which can be added in a different PR at a later date.

I hope we can start iterating comments over this PR and add this great language in!

@github-actions github-actions bot added c++ CI Continuous Integration codegen Involving generating code from schema labels Sep 15, 2022
dbaileychess
dbaileychess reviewed Sep 16, 2022
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
reflection/reflection.fbs Outdated Show resolved Hide resolved
tests/FlatBuffers.Test.Nim/tests/moredefaults/test.nim Outdated Show resolved Hide resolved
tests/FlatBuffers.Test.Nim/NimTest.sh Outdated Show resolved Hide resolved
nim/flatbuffers.nimble Outdated Show resolved Hide resolved
@github-actions github-actions bot added the lua label Sep 16, 2022
CasperN
CasperN reviewed Sep 19, 2022
View reviewed changes
src/bfbs_gen.h Show resolved Hide resolved
src/bfbs_gen_nim.cpp Outdated Show resolved Hide resolved
@danlapid
Copy link
Contributor Author

indeed, anything that can be done about it?

Pinging would work, but also submitting a PR also works - the restriction is only for first time contributors. The fix to Type::Serialize seems important to think about and a good candidate to split out into a small PR.

I doubt I will need many more rounds of ci before this works (hopefully next run will work, the general tasks are passing, only my new test nim task is giving me issues that I hope are solved).
Considering that I'd rather not indulge in another PR unless you deem it a necessary split.

@danlapid
Copy link
Contributor Author

@CasperN @dbaileychess I’d love a ci run whenever you’re available, Thanks

@danlapid
Copy link
Contributor Author

@dbaileychess @CasperN
Seems like the CI is finally green (currently yellow just because I merged with master to keep the branch up to date).
Any other comments regarding the implementation?

CasperN
CasperN reviewed Sep 27, 2022
View reviewed changes
src/bfbs_gen_nim.cpp Outdated Show resolved Hide resolved
src/bfbs_gen_nim.cpp Show resolved Hide resolved
src/bfbs_gen_nim.cpp Outdated Show resolved Hide resolved
src/bfbs_gen_nim.cpp Outdated Show resolved Hide resolved
src/bfbs_gen_nim.cpp Outdated Show resolved Hide resolved
src/bfbs_gen_nim.cpp Show resolved Hide resolved
@danlapid
Copy link
Contributor Author

@CasperN Pulled and merged with master, no conflicts and tests are passing.
Anything else we want to change regarding this PR?

@dbaileychess dbaileychess added the release-notes The PR should be highlighted in the release notes of the next release it is in. label Oct 19, 2022
@dbaileychess
Copy link
Collaborator

@danlapid I would like to include this in the next version release. Looks like @CasperN had one open converstation to resolve.

@danlapid
Copy link
Contributor Author

@danlapid I would like to include this in the next version release. Looks like @CasperN had one open converstation to resolve.

I’ll take care of it today and ping you guys

@danlapid
Copy link
Contributor Author

@CasperN @dbaileychess Added the requested code.

CasperN
CasperN reviewed Oct 21, 2022
View reviewed changes
Copy link
Collaborator

@CasperN CasperN left a comment

Choose a reason for hiding this comment

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

Hi, I still have a minor comment and question about where certain code should live

src/namer.h Outdated Show resolved Hide resolved
tests/nim/testnim.py Outdated Show resolved Hide resolved
@danlapid
Copy link
Contributor Author

@CasperN Fixed the issues you noted in your last 2 comments.

@CasperN
Copy link
Collaborator

CasperN commented Oct 21, 2022

LGTM!

@CasperN CasperN merged commit 872a497 into google:master Oct 21, 2022
bulentv pushed a commit to zdrealityhub/flatbuffers that referenced this pull request Oct 21, 2022
@danlapid @dbaileychess @bulentv
[Nim] Bfbs Nim Generator (google#7534)
7b1091e 
* Bfbs Nim Generator

* Remove commented out tests

* add missing line to idl.h

* Commit python reflection changes

* Commit python reflection changes and move tests

* Remove default string addition

* Move tests to python file

* Fix element size check when element is table

* remove whitespace changes

* add element_type docs and commit further to namer and remove kkeep

* Bfbs Nim Generator

* Remove commented out tests

* add missing line to idl.h

* Commit python reflection changes

* Commit python reflection changes and move tests

* Remove default string addition

* Move tests to python file

* Fix element size check when element is table

* remove whitespace changes

* add element_type docs and commit further to namer and remove kkeep

* remove unused variables

* added tests to ci

* added tests to ci

* fixes

* Added reflection type Field, Variable to namer

* Moved reflection namer impl to bfbsnamer

* Remove whitespace at end of line

* Added nim to generated code

* Revert whitespace removal

Co-authored-by: Derek Bailey <derekbailey@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CasperN CasperN CasperN left review comments

@dbaileychess dbaileychess dbaileychess approved these changes

Assignees
No one assigned
Labels
c++ CI Continuous Integration codegen Involving generating code from schema lua python release-notes The PR should be highlighted in the release notes of the next release it is in.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@danlapid @CasperN @dbaileychess