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

Use private thrift members #3405

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Conversation

ctubbsii
Copy link
Member

This is a work in progress towards using private thrift members

@ctubbsii ctubbsii linked an issue May 16, 2023 that may be closed by this pull request
@DomGarguilo
Copy link
Member

While fixing some of these, I noticed a typo in TConstraintViolationSummary with member variable "constrainClass" which should be "constraintClass". Not sure that this is the best place to fix it but I wanted to make a note of it somewhere so I don't forget.

@keith-turner
Copy link
Contributor

What was the process that was used to generate these changes?

@dlmarion
Copy link
Contributor

What was the process that was used to generate these changes?

See the change to generate-thrift.sh

@keith-turner
Copy link
Contributor

What was the process that was used to generate these changes?

See the change to generate-thrift.sh

That was helpful. Was any automation used to modify the non thrift java code?

@ctubbsii
Copy link
Member Author

ctubbsii commented Jun 2, 2023

What was the process that was used to generate these changes?

See the change to generate-thrift.sh

That was helpful. Was any automation used to modify the non thrift java code?

I haven't used any. I'm looking at things on a case-by-case basis and working through incrementally. @DomGarguilo has also contributed by issuing PRs against my branch, but I haven't finished merging those into this WIP branch yet.

These non-thrift Java code will need review as we make progress on this. It's tedious to migrate the code, and tedious to review. I don't see any automated way around it.... unfortunately.

@keith-turner
Copy link
Contributor

These non-thrift Java code will need review as we make progress on this. It's tedious to migrate the code, and tedious to review. I don't see any automated way around it.... unfortunately.

Ok. The reason I was asking was because if some automated process was being used I would rather review that process than the actual changes. The only thing I can think for automating this is something like the following.

  1. In IDE before regenerating the thrift type, for each variable in the thrift type refactor the variable in the thrift code to use getters and setters.
  2. Regenerate the thrift code w/ getters and setters for the type changed above.
  3. See if it compiles.

@ctubbsii
Copy link
Member Author

ctubbsii commented Jun 2, 2023

These non-thrift Java code will need review as we make progress on this. It's tedious to migrate the code, and tedious to review. I don't see any automated way around it.... unfortunately.

Ok. The reason I was asking was because if some automated process was being used I would rather review that process than the actual changes. The only thing I can think for automating this is something like the following.

  1. In IDE before regenerating the thrift type, for each variable in the thrift type refactor the variable in the thrift code to use getters and setters.
  2. Regenerate the thrift code w/ getters and setters for the type changed above.
  3. See if it compiles.

That might be possible. I'm not sure if there's an easy way to find/replace these with getters/setters, and a lot of it requires manual intervention anyway (use of getListSize() instead of getList().size() or using the variable type as boolean to use "is" instead of "get", and primitive setters can't use increment, and verifying that null values is the same as "is not set" instead of "is set to null"). It's probably safer to review each compilation failure on a case-by-case basis and fix each one manually, than to try an automated procedure that's going to miss things. As the reviewer, it's probably the same outcome (because you don't know what the implementer considered or overlooked), but for the person implementing it, I think they'll likely catch more things the automation would have missed.

Copy link
Member

@DomGarguilo DomGarguilo left a comment

Choose a reason for hiding this comment

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

I took a look at all the manually changed files and everything looks good.

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.

Generate thrift with private members
4 participants