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

Closes #31: Add lint for fields-only classes. #32

Merged
merged 1 commit into from
Nov 29, 2018

Conversation

agi90
Copy link
Contributor

@agi90 agi90 commented Nov 28, 2018

This adds a lint to make sure that classes that only contain final fields have
a protected or public constructor in the API so that external clients can mock
these classes. It also checks that these classes are not final for test
subclassing.

This adds a lint to make sure that classes that only contain final fields have
a protected or public constructor in the API so that external clients can mock
these classes. It also checks that these classes are not final for test
subclassing.
@agi90 agi90 requested a review from snorp November 28, 2018 19:40
@snorp
Copy link
Contributor

snorp commented Nov 29, 2018

I'm ok with this, but it seems like it may be a little specific to our use case. Maybe it would be better suited as a rule in some other linter?

@agi90
Copy link
Contributor Author

agi90 commented Nov 29, 2018

I'm ok with this, but it seems like it may be a little specific to our use case. Maybe it would be better suited as a rule in some other linter?

Right now none of the lints actually stop you from updating the api file and ignoring them so I think it's fine to merge this as is. As discussed on slack, in the future we'll be able to select which lints run so that should ease your concern, opened Issue #34 for this.

@agi90 agi90 merged commit d7d9211 into mozilla-mobile:master Nov 29, 2018
@agi90 agi90 deleted the field-only branch November 29, 2018 15:40
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

3 participants