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 support for Jar to Jar transformation to JarInfer #316

Merged
merged 17 commits into from May 28, 2019

Conversation

navahgar
Copy link
Contributor

@navahgar navahgar commented May 24, 2019

This adds support for annotating jar files, as opposed to creating an astubx file on the side.

Summary of changes:

  • Added support to annotate jar files with the annotations inferred by the analysis in JarInfer.
  • Added a flag in JarInfer CLI to enable annotating jar files.
  • Tested annotated jar files by checking for expected annotations.
  • Added the expected annotations in the sources used for tests.

@navahgar navahgar requested a review from lazaroclapp May 24, 2019 17:07
Copy link
Collaborator

@lazaroclapp lazaroclapp left a comment

Choose a reason for hiding this comment

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

Lots of comments because this is lots of code, but overall the approach and code looks good to me.

A few general points:

  • Not sure I understand the logic for the source based test cases (the one using a test project I get, just the one involving getToyTestSrcWithExpectAnnotations doesn't quite make sense to me.
  • Please change the PR title and description to something that would make for a useful documenting commit message once we squash and merge. Otherwise we'll be scratching our heads a year from now trying to figure out what JarInfer (#316) was ;)
  • Probably worth adding a few javadocs at the class and public method level
  • Also, I just realized this is missing in some other files, but the license information should be at the top of each source files, as in https://github.com/uber/NullAway/blob/master/nullaway/src/main/java/com/uber/nullaway/NullAway.java (will send you the internal guidelines in a moment)

@navahgar
Copy link
Contributor Author

Thanks for the detailed comments. Addressed them. PTAL.

Copy link
Contributor

@kageiit kageiit left a comment

Choose a reason for hiding this comment

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

Can you please add a more descriptive title and a quick summary of the top changes maybe to now we changed jarinfer

@navahgar navahgar changed the title Jarinfer Adding support for annotating jar files with the annotations inferred from the analysis in JarInfer. May 28, 2019
@navahgar
Copy link
Contributor Author

Summary of changes:

  • Added support to annotate jar files with the annotations inferred by the analysis in JarInfer.
  • Added a flag in JarInfer CLI to enable annotating jar files.
  • Tested annotated jar files by checking for expected annotations.
  • Added the expected annotations in the sources used for tests.

@navahgar
Copy link
Contributor Author

Can you please add a more descriptive title and a quick summary of the top changes maybe to now we changed jarinfer

Updated the title and added a summary.

Copy link
Collaborator

@lazaroclapp lazaroclapp left a comment

Choose a reason for hiding this comment

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

LGTM, see comment about missing Javadoc, and also please make sure the title of the commit is under 80 characters (we don't always follow this, but we probably should :), you can still have a longer explanation inside the message body).

Suggestion:

Add support for Jar to Jar transformation to JarInfer

This adds support for annotating jar files, as opposed to creating an 
astubx file on the side. 

Summary of changes:
...

Followed for the stuff you already have. Once those two are addressed, feel free to land it.

Copy link
Collaborator

@msridhar msridhar left a comment

Choose a reason for hiding this comment

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

Just a code style comment that probably should be fixed in a follow-up rather than here.

@navahgar navahgar changed the title Adding support for annotating jar files with the annotations inferred from the analysis in JarInfer. Add support for Jar to Jar transformation to JarInfer May 28, 2019
@navahgar navahgar merged commit 7a29a31 into uber:master May 28, 2019
@navahgar navahgar deleted the jarinfer branch May 29, 2019 18:11
@lazaroclapp lazaroclapp mentioned this pull request Jan 6, 2020
3 tasks
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.

None yet

4 participants