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

Enable lint #4588

Closed
wants to merge 2 commits into from
Closed

Enable lint #4588

wants to merge 2 commits into from

Conversation

apoorv-arora
Copy link

@apoorv-arora apoorv-arora commented Oct 21, 2020

What is it?

  • Codebase improvement (dev facing)

Description of the changes in your PR

Current project has lint disabled. As a result of which, any new commit which is scheduled to go live may not pass the list of checks provided by android lint. To learn the list of checks that are done by lint, navigate here.

This PR enables lint workflow integration by creating a baseline file which includes all errors (1) & warnings (89) in the current project & treat them as a baseline. Any new commit will now throw an error if they do not follow the lint standard guideline.

Implementation

  1. ./gradlew lintDebug

@apoorv-arora apoorv-arora changed the title App lint Enable lint Oct 22, 2020
@TheAssassin
Copy link
Member

I'm not sure if merging is a good idea. You're adding a file with 27k lines of XML. It's pretty much impossible to review this. You don't explain how you added created this file (I'm pretty sure you didn't hand craft it), so it remains unclear how this linting system would have to be maintained in the future by the team.

Please elaborate. Also, it would be better to open an issue first which you can then reference here. Please create one. Quoting the contribution guidelines:

If you want to add a feature or change one, please open an issue describing your change. This gives the team and community a chance to give feedback before you spend any time on something that could be done differently or not done at all. It also prevents two contributors from working on the same thing and one being disappointed when only one user's code can be added.

@TheAssassin TheAssassin added the deep review required Maintainer must double check/test/review this due to changes in API or architecture label Oct 22, 2020
@apoorv-arora
Copy link
Author

I'm not sure if merging is a good idea. You're adding a file with 27k lines of XML. It's pretty much impossible to review this. You don't explain how you added created this file (I'm pretty sure you didn't hand craft it), so it remains unclear how this linting system would have to be maintained in the future by the team.

Please elaborate. Also, it would be better to open an issue first which you can then reference here. Please create one. Quoting the contribution guidelines:

If you want to add a feature or change one, please open an issue describing your change. This gives the team and community a chance to give feedback before you spend any time on something that could be done differently or not done at all. It also prevents two contributors from working on the same thing and one being disappointed when only one user's code can be added.

We can take a snapshot of the project's current set of warnings & errors specified by lint, and then use the snapshot as a baseline for future inspection runs so that only new issues are reported. The baseline snapshot will let us start using lint to fail the build without having to go back and address all existing issues first. All the subsequent checks are will have to go through lint now.
By specifying the file name in build.gradle, & running (./gradlew lintDebug) we can generate the baseline file.

@Stypox
Copy link
Member

Stypox commented Oct 22, 2020

Please fix the errors and warnings instead of adding them to the blacklist: that's what pther PRs adding lint did.

@TobiGr TobiGr force-pushed the dev branch 2 times, most recently from 679bc75 to 2aeccc0 Compare March 16, 2021 08:24
@litetex litetex marked this pull request as draft October 1, 2021 17:44
@litetex
Copy link
Member

litetex commented Oct 1, 2021

Closing this for now:

  • no progress
  • no GitHub actions build
  • merge conflicts

Feel free to reopen it when there is progress again.

@litetex litetex closed this Oct 1, 2021
@litetex litetex mentioned this pull request Oct 10, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deep review required Maintainer must double check/test/review this due to changes in API or architecture
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants