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

Removed synthentic accessor from auto disposables and recording observer #103

Merged
merged 1 commit into from
Oct 8, 2017
Merged

Removed synthentic accessor from auto disposables and recording observer #103

merged 1 commit into from
Oct 8, 2017

Conversation

bangarharshit
Copy link
Contributor

Removed synthetic accessors. Fixes - #102
Total dex count

  • autodispose package: Master - 454, New - 449,
  • recording observer - Master - 49, New - 42

@CLAassistant
Copy link

CLAassistant commented Oct 8, 2017

CLA assistant check
All committers have signed the CLA.

@ZacSweers
Copy link
Collaborator

This seems fine to me. Mind adding a comment on those methods to indicate that they're normally private but made package-local for the accessors?

Just adding something like an inline /* private */ might be enough context for now. I've proposed an annotation in error-prone for enforcement of this - google/error-prone#765

@bangarharshit
Copy link
Contributor Author

Added comments to indicate the scope.

@bangarharshit
Copy link
Contributor Author

bangarharshit commented Oct 8, 2017

Making it inline.

@ZacSweers
Copy link
Collaborator

Make sure you sign the correct CLA, it seems you're committing from a different github account

Added comment to annotate that the method is private
@bangarharshit
Copy link
Contributor Author

Fixed the account for commit.

Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

Thanks! I wonder if we can have a tool automate checking this.

Also in the future, mind keeping follow-up commits separate rather than squashing? Makes it easier to review follow-ups in isolation.

@ZacSweers ZacSweers merged commit 1c94067 into uber:master Oct 8, 2017
@bangarharshit
Copy link
Contributor Author

Sure. Will take care of it.

IntelliJ provides inspection for these but yeah some kind of error-prone check on CI will be great. Maybe a custom checker if the repo supports that.

@bangarharshit bangarharshit deleted the synthetic-accessor branch October 8, 2017 11:04
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.

3 participants