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 browse for proto imports and other minor cleanup #325

Merged
merged 6 commits into from
Dec 8, 2023

Conversation

jasonmreding
Copy link
Collaborator

This addresses #186. It also includes changes related to #313 and #38.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only change here is hidden in right click configuration options of the path control(s) used to specify directories from which to find imported proto files. The path control is now configured to browse for directories instead of files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only change here is to separate compiled code from source.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only change here is to separate compiled code from source.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I started making changes to this and Create Oneof Message Converters.vi as I was debugging and trying to improve code generation behavior when regenerating code in project in which previously generated code already exists. However, I soon realized making this developer flow work well probably requires an extensive overhaul of the existing code, which is more than I have time for at the moment. However, I decided to submit these VIs anyway since they have been simplified quite a bit, and are much easier to read.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This VI didn't think it belonged to the library for some reason. Now it does.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also modified this while trying to improve behavior of regenerating a proto file without first deleting everything that was previously generated. This change prevents multiple copies of the top level enum from getting added to the typedef, which breaks the typedef. Having a broken typedef in turn generates lots of downstream errors with VI scripting when adding things by path and what not.

@jasonmreding
Copy link
Collaborator Author

@pratheekshasn Can you review this PR and/or assign to appropriate reviewers. It appears I don't have write access so I can't add reviewers myself. Would it also be possible for me to get write access to the repo?

@pratheekshasn pratheekshasn merged commit 378b486 into ni:master Dec 8, 2023
5 checks passed
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