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

Fix problems on CI, conventions are not checked. #173

Merged
merged 6 commits into from Sep 2, 2019
Merged

Fix problems on CI, conventions are not checked. #173

merged 6 commits into from Sep 2, 2019

Conversation

loechel
Copy link
Member

@loechel loechel commented Aug 30, 2019

On CI (Travis and Appveyor) the lint Checks does not run and therefore the master is dirty.

This Pull-Request is a base to fix the dirty Master and ensure that the Linting ( Convention Checks ) are run on each push to github. This fixes #172

By the way therefor this Pull Request should be red on CI to show that the errors now show up.

As a second step then a fix to the dirty master is necessary to fix the lining errors.

On CI (Travis and Appveyor) the lint Checks does not run and therefore the master is dirty.
@loechel loechel added the bug label Aug 30, 2019
@loechel loechel self-assigned this Aug 30, 2019
@loechel loechel changed the title fix problems in CI for conventions. Fix problems on CI, conventions are not checked. Aug 30, 2019
loechel added a commit that referenced this pull request Sep 1, 2019
@icemac
Copy link
Member

icemac commented Sep 2, 2019

I did it the other way round: Merging #174 first and updating this PR to the new master to get it green.

@icemac icemac added this to In progress in Zope 4 bugfix via automation Sep 2, 2019
Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

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

Mostly LGTM the only change request I have is a change which does not seem to belong to the core of this PR and where I disagree with the change.

tox.ini Outdated
@@ -48,10 +48,9 @@ commands =
coverage combine
coverage html
coverage xml
coverage report --skip-covered --show-missing --fail-under=100.0
coverage report --show-missing --fail-under=100.0
Copy link
Member

Choose a reason for hiding this comment

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

I do not understand why you want to see a long list of 100 % covered modules as 100 % coverage is required. If a module drops below it will be shown as the only one with too low coverage no need to search for the module in a long output.

Suggested change
coverage report --show-missing --fail-under=100.0
coverage report --skip-covered --show-missing --fail-under=100.0

Copy link
Member Author

Choose a reason for hiding this comment

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

well I do understand that you like to only show the files where actual coverage is missing.
I have reactivate that setting but in the setup.cfg file

Copy link
Member

Choose a reason for hiding this comment

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

Nice idea.

@loechel loechel requested a review from icemac September 2, 2019 13:20
@loechel loechel merged commit 64f238e into master Sep 2, 2019
@loechel loechel deleted the fix-ci branch September 2, 2019 16:19
Zope 4 bugfix automation moved this from In progress to Done Sep 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Zope 4 bugfix
  
Done
Development

Successfully merging this pull request may close these issues.

master branch currently dirty
2 participants