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

Feature: auto-fix for specific linters #439

Merged
merged 31 commits into from
Apr 23, 2022
Merged

Feature: auto-fix for specific linters #439

merged 31 commits into from
Apr 23, 2022

Conversation

phamelink
Copy link
Contributor

@phamelink phamelink commented Apr 14, 2022 •
edited by ocean90

Fixes #426.

With @BastienFaivre, we added a feature to enable auto fix for specific linters requested in this issue #426.

We made several attempts at this, starting with using <linter>_auto_fix to only commit and push certain file extensions found in the input for each linter. When we realised that this wasn't very efficient as there could be edge cases where this would cause problems if some linters have common file extensions, we tried another way, where we would restore the branch at the start of every linter iteration. Finally, we realised that this actually wasn't necessary, as we could directly prevent a linter from auto-fixing by using autoFix && linterAutoFix as the fix parameter in a linter's lint function.

We also made sure to add documentation about this feature in the README and made an example where this could be used (typically for linters like flake8 which do not support auto-fixing).

We tested these changes on this repo, and it seems to be working as expected.

phamelink and others added 24 commits April 7, 2022 17:00
@phamelink
Add <linter>_auto_fix for each linter in action.yml
d18da1e
@phamelink
Update test.yml
3f2ab84
@phamelink
Update test-action.yml
0fbbf58
@phamelink
Update test-action.yml
1ee9566
@phamelink
Update index.js
2738ec2
@phamelink
Update git.js
90f43c2
@phamelink
Merge branch 'master' of github.com:wearerequired/lint-action into we…
2462d8d 
…arerequired-master

# Conflicts:
#	yarn.lock
@phamelink
Merge branch 'wearerequired-master'
6bb1cf0
@phamelink
Merge branch 'master' of github.com:phamelink/lint-action
ddc8753 
* 'master' of github.com:phamelink/lint-action:
  Update git.js
  Update index.js
  Update test-action.yml
  Update test-action.yml
  Update test.yml
@phamelink
no message
70aae93
@phamelink
Feature: <linter_id>_auto_fix to select which linters you want to aut…
1cd4c21 
…o fix by restoring changes made to the current branch to avoid committing and pushing modifications made by previous linters.
@phamelink
Add eslint
ce76e33
@phamelink
Fix restore files in case there are no changes
763aefd
@phamelink
Fix restore files in case there are no changes
bb769c0
@phamelink
Fix restore args
7c8c570
@phamelink
Fix git restoreFiles
292636b
@phamelink
Merge pull request #5 from phamelink/implementation-restore
4247ce5 
Feature: <linter_id>_auto_fix
@phamelink
Added core info debugging
c7363ab
@phamelink
Commit build
aaf4231
@phamelink
Removed restore changes
6dc8acf
@phamelink
Removed export error
d2293fd
@phamelink
Fix README typo and added example of auto_fix
ad9eb50
@phamelink
Reset some files to original
972493a
@phamelink
Add build
7d6d45e
phamelink added a commit to phamelink/devops-course that referenced this pull request 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).
ocean90
ocean90 reviewed Apr 14, 2022
View reviewed changes
dist/index.js Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
@BastienFaivre BastienFaivre mentioned this pull request Apr 14, 2022
Merged
This was referenced Apr 16, 2022
Merged
Merged
khaes-kth pushed a commit to KTH/devops-course that referenced this pull request 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>
ocean90
ocean90 requested changes Apr 22, 2022
View reviewed changes
Copy link
Member

@ocean90 ocean90 left a comment

Choose a reason for hiding this comment

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

This is looking good, just one minor doc change.

README.md Outdated Show resolved Hide resolved
phamelink and others added 2 commits April 22, 2022 12:55
@phamelink @ocean90
Update docs
ec64a46 
Co-authored-by: Dominik Schilling <dominikschilling+git@gmail.com>
@ocean90
Merge branch 'master' into phamelink/master
b8486a2
@ocean90 ocean90 merged commit 2721407 into wearerequired:master Apr 23, 2022
@ocean90
Copy link
Member

ocean90 commented Apr 23, 2022

Nice job, thank you!

@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.
Reviewers

@Gismo359 Gismo359 Gismo359 left review comments

@ocean90 ocean90 ocean90 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Suggestion: option to enable auto fix for specific linters
4 participants
@phamelink @ocean90 @Gismo359 @BastienFaivre