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.

Sign up for GitHub

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

Jump to bottom

Suggestion: option to enable auto fix for specific linters #426

Closed
codeman1o1 opened this issue Mar 30, 2022 · 4 comments · Fixed by #439
Closed

Suggestion: option to enable auto fix for specific linters #426

codeman1o1 opened this issue Mar 30, 2022 · 4 comments · Fixed by #439
Labels
enhancement New feature or request help wanted Extra attention is needed stale

Comments

@codeman1o1
Copy link
Contributor

Some linters do not support auto fix, maybe this will be a fix

Something like:
black_auto_fix: true
@ocean90 ocean90 added enhancement New feature or request help wanted Extra attention is needed labels Apr 4, 2022
@ocean90
Copy link
Member

ocean90 commented Apr 4, 2022

Sounds like a good idea. I'd probably keep the global auto_fix setting and then allow to deactivate the fixer for specific linters via <linter>_auto_fix: false.

@phamelink phamelink mentioned this issue Apr 6, 2022
Merged
@phamelink
Copy link
Contributor

Hi @ocean90 , @BastienFaivre and I are students currently studying DevOps at KTH. One of our assignments is to contribute to an open-source project related to DevOps, and as big linter fans we found your action to be a perfect fit! We plan on making a pull request within the next 10 days to add this feature!

We'll follow on what you said to keep the global auto_fix setting and add make sure to add the inputs <linter>_auto_fix with a default set to true, so that when we loop over each linter, we only commit changes when both auto_fix and <linter>_auto_fix are set to true. We would add in the description of each input that if set to true, it will only auto-fix a specific linter if the global auto_fix is set to true to avoid any confusion for users.

We'll also suggest an addition to the README to mention this new feature.

With regards to writing tests, since this is a rather simple feature to add, is there anything in particular you would want us to add ?

Let us know if you any comments/suggestions!

@BastienFaivre BastienFaivre mentioned this issue Apr 9, 2022
Closed
@phamelink phamelink mentioned this issue Apr 14, 2022
Merged
phamelink added a commit to phamelink/devops-course that referenced this issue Apr 14, 2022
@phamelink
Submission: Open source contribution
68a118e 
# Assignment Proposal

## Title

Open source contribution on a [Lint Action](https://github.com/marketplace/actions/lint-action), an github action that shows linting errors and allows auto-fixing issues.

## Names and KTH ID
  - Philip Hamelink (ppjhad@kth.se)
  - Bastien Faivre (faivre@kth.se)
## Deadline

Task 2: April 19, 17h

## Category

Contribution to open-source

## Description

We want to add a feature mentioned in this [issue](wearerequired/lint-action#426). Currently, this action allows to set 
auto-fixing linting errors for all linters. There has been a requested feature to only allow auto-fixing for certain linters, which we want to
add ourselves. The repo has 329 ⭐ and 509 commits. The community is quite active. The issue was requested 7 days ago, and the author responded and 
labeled the issue with "help_wanted" 2 days ago. 

We believe this satisfies the requirements for the open-source contribution assignment as linting code on commits is an invaluabe feature in DevOps.

Submission:

In our [pull request](wearerequired/lint-action#439), we mention the issues we have had, and how we have come up with our final solution. We tested our new feature [here](https://github.com/phamelink/test-action-repo), where you can see the results from our action where we can specify which linters we want to automatically fix code issues.  

We believe we satisfy the following grading criteria:

|                                             | Yes | No | 
|-------------------------------------------- | ----|----|
|bug: The contribution fixes bugs | Yes ✅ | No | 
|documentation: The contribution improves documentation | Yes ✅ | No |
|feat: The contribution adds new features | Yes ✅ | No |
|difficulty: The contribution is a difficult piece of engineering | Yes ✅ | No | 
|conversation: There is an interesting engineering conversation with the maintainers | Yes | No | 
|merge: The contribution is merged in the main branch.| Yes | No | 

We believe we fixed a small bug. Before, when auto-fix was set, when linters did not support auto-fixing there was a warning that could not be prevented. This can now be avoided when the input <linter>_auto_fix is set to `false` for a non auto-fix supporting linter. 
  
We improved documentation by adding information about our new feature (since there are new inputs in our version), and we also added an example for our feature.
  
This contribution adds an obvious new feature as requested.
  
Finally, we believe that even though the final changes were minimal and quite simple, this contribution was quite challenging as we had to figure out exactly where and when we needed to add a condition. We went through many stages, most involving manipulations with git to avoid committing and pushing certain files as was tried [here](https://github.com/phamelink/lint-action/tree/implementation) or with this [script](https://github.com/phamelink/lint-action/blob/test-branch/t.sh), or even restore files to their original state done in this [commit](phamelink/lint-action@1cd4c21).
This was referenced Apr 16, 2022
Merged
Merged
khaes-kth pushed a commit to KTH/devops-course that referenced this issue Apr 20, 2022
@phamelink @BastienFaivre
Open source Submission (#1798)
15c3dfa 
* Proposal: Contribution to open-source

* Submission: Open source contribution

# Assignment Proposal

## Title

Open source contribution on a [Lint Action](https://github.com/marketplace/actions/lint-action), an github action that shows linting errors and allows auto-fixing issues.

## Names and KTH ID
  - Philip Hamelink (ppjhad@kth.se)
  - Bastien Faivre (faivre@kth.se)
## Deadline

Task 2: April 19, 17h

## Category

Contribution to open-source

## Description

We want to add a feature mentioned in this [issue](wearerequired/lint-action#426). Currently, this action allows to set 
auto-fixing linting errors for all linters. There has been a requested feature to only allow auto-fixing for certain linters, which we want to
add ourselves. The repo has 329 ⭐ and 509 commits. The community is quite active. The issue was requested 7 days ago, and the author responded and 
labeled the issue with "help_wanted" 2 days ago. 

We believe this satisfies the requirements for the open-source contribution assignment as linting code on commits is an invaluabe feature in DevOps.

Submission:

In our [pull request](wearerequired/lint-action#439), we mention the issues we have had, and how we have come up with our final solution. We tested our new feature [here](https://github.com/phamelink/test-action-repo), where you can see the results from our action where we can specify which linters we want to automatically fix code issues.  

We believe we satisfy the following grading criteria:

|                                             | Yes | No | 
|-------------------------------------------- | ----|----|
|bug: The contribution fixes bugs | Yes ✅ | No | 
|documentation: The contribution improves documentation | Yes ✅ | No |
|feat: The contribution adds new features | Yes ✅ | No |
|difficulty: The contribution is a difficult piece of engineering | Yes ✅ | No | 
|conversation: There is an interesting engineering conversation with the maintainers | Yes | No | 
|merge: The contribution is merged in the main branch.| Yes | No | 

We believe we fixed a small bug. Before, when auto-fix was set, when linters did not support auto-fixing there was a warning that could not be prevented. This can now be avoided when the input <linter>_auto_fix is set to `false` for a non auto-fix supporting linter. 
  
We improved documentation by adding information about our new feature (since there are new inputs in our version), and we also added an example for our feature.
  
This contribution adds an obvious new feature as requested.
  
Finally, we believe that even though the final changes were minimal and quite simple, this contribution was quite challenging as we had to figure out exactly where and when we needed to add a condition. We went through many stages, most involving manipulations with git to avoid committing and pushing certain files as was tried [here](https://github.com/phamelink/lint-action/tree/implementation) or with this [script](https://github.com/phamelink/lint-action/blob/test-branch/t.sh), or even restore files to their original state done in this [commit](phamelink/lint-action@1cd4c21).

Co-authored-by: Bastien Faivre <57015770+BastienFaivre@users.noreply.github.com>
@github-actions
Copy link
Contributor

A stale label has been added to this issue because it has been open 15 days with no activity. To keep this issue open, add a comment within 5 days.

@github-actions github-actions bot added the stale label Apr 23, 2022
@codeman1o1
Copy link
Contributor Author

I demand that you remove the "stale" label

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
enhancement New feature or request help wanted Extra attention is needed stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Feature: auto-fix for specific linters phamelink/lint-action
3 participants
@ocean90 @codeman1o1 @phamelink