Only run AppVeyor linting check on branches which aren't master - #12378
Conversation
seanbudd
changed the title
|
|
||
| # Flake8 Linting | ||
| - ps: | | ||
| if ($env:APPVEYOR_REPO_BRANCH -ne "master") { |
There was a problem hiding this comment.
Doesn't this also need to be done for any other build that doesn't have a "target": EG release builds / beta builds / try build?
There was a problem hiding this comment.
Originally, we only did linting for PRs. I'm guessing the aim recently was to also enable linting for at least try builds. Personally, I think we only need lint checks for prs and nothing more, as it is prs by which code gets into NVDA.
However, if there is a strong argument for also enabling linting for try builds, then the if check should be pretty much that it happens for prs (APPVEYOR_PULL_REQUEST_NUMBER is defined) or APPVEYOR_REPO_BRANCH starts with "try-". Anything else should not do linting.
We do not need to do linting on official branches or releases as code can only get to them from prs.
There was a problem hiding this comment.
Yes, I guess I wasn't clear enough. I meant we should avoid linting release builds / beta / try builds. It only makes sense to lint PR builds.
There was a problem hiding this comment.
Isn't helpful to know if a try-build can be linted when merged with master? I didn't see a reason to why it would be specifically excluded if the aim is to land code on master (eventually).
There was a problem hiding this comment.
There are different reasons for a try build. It may just be a prototype, in this case a failed lint is just a distraction. If the goal is to get the code merged, the same commit will likely be used as the HEAD on the PR branch, which should get a lint.
feerrenrut
left a comment
feerrenrut
left a comment
There was a problem hiding this comment.
I'd say we merge this now, to get alpha builds passing again. Happy to have a followup on the other build types if necessary later.
|
|
||
| # Flake8 Linting | ||
| - ps: | | ||
| if ($env:APPVEYOR_REPO_BRANCH -ne "master") { |
There was a problem hiding this comment.
Originally, we only did linting for PRs. I'm guessing the aim recently was to also enable linting for at least try builds. Personally, I think we only need lint checks for prs and nothing more, as it is prs by which code gets into NVDA.
However, if there is a strong argument for also enabling linting for try builds, then the if check should be pretty much that it happens for prs (APPVEYOR_PULL_REQUEST_NUMBER is defined) or APPVEYOR_REPO_BRANCH starts with "try-". Anything else should not do linting.
We do not need to do linting on official branches or releases as code can only get to them from prs.
3 participants
Link to issue number:
None
Summary of the issue:
Master builds are currently failing at the linting step such as
Lint checking doesn't need to occur for master builds
Description of how this pull request fixes the issue:
Don't run the appveyor lint checking on master branches
Testing strategy:
Known issues with pull request:
Change log entries:
None
Code Review Checklist: