Skip to content

Conversation

@Borda
Copy link
Collaborator

@Borda Borda commented Jan 1, 2021

What does this PR do?

adding to-do notes for exceptions that shall be better specified and cleaning a few of them...

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes [if needed]?
  • Did you write any new necessary tests [no need for typos, docs]?
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified
  • Check that target branch and milestone are aligned!

Did you have fun?

Make sure you had fun coding 🙃

@Borda Borda added the refactor label Jan 1, 2021
@Borda Borda added this to the 1.2 milestone Jan 1, 2021
@codecov
Copy link

codecov bot commented Jan 1, 2021

Codecov Report

Merging #5320 (9b79d27) into release/1.2-dev (9dbdffc) will decrease coverage by 1%.
The diff coverage is 45%.

@@               Coverage Diff                @@
##           release/1.2-dev   #5320    +/-   ##
================================================
- Coverage               93%     91%    -1%     
================================================
  Files                  146     146            
  Lines                10211   10430   +219     
================================================
+ Hits                  9486    9535    +49     
- Misses                 725     895   +170     

@Borda Borda force-pushed the refactor/exceptions branch from 14f39c1 to bc52689 Compare January 1, 2021 18:37
@Borda Borda marked this pull request as ready for review January 1, 2021 18:53
@Borda Borda enabled auto-merge (squash) January 1, 2021 21:56
@carmocca
Copy link
Contributor

carmocca commented Jan 2, 2021

Is there a CI test to check that new code does not introduce broad exception catching?

@Borda
Copy link
Collaborator Author

Borda commented Jan 2, 2021

Is there a CI test to check that new code does not introduce broad exception catching?

that would be nice, but not sure which tool does it... @carmocca mind have look?

@carmocca
Copy link
Contributor

carmocca commented Jan 2, 2021

Do we not run CodeFactor anymore?

@Borda
Copy link
Collaborator Author

Borda commented Jan 2, 2021

Do we not run CodeFactor anymore?

we do run it, so you suggest to define it there?

@Borda Borda self-assigned this Jan 2, 2021
@Borda Borda requested a review from carmocca January 2, 2021 15:50
@Borda Borda added the ready PRs ready to be merged label Jan 2, 2021
@carmocca
Copy link
Contributor

carmocca commented Jan 2, 2021

we do run it, so you suggest to define it there?

Mind linking where it is run, I can't to find it? If I'm not mistaken, it has a rule for "Broad Exception catching", so it should appear as a failing check.

@Borda
Copy link
Collaborator Author

Borda commented Jan 2, 2021

Mind linking where it is run, I can't to find it?

it seems it is running only for master...

If I'm not mistaken, it has a rule for "Broad Exception catching", so it should appear as a failing check.

there is no such a setting, probably a different tool, I remember I saw it elsewhere...
Screenshot 2021-01-03 at 0 34 48

@Borda Borda requested a review from carmocca January 2, 2021 23:36
@carmocca
Copy link
Contributor

carmocca commented Jan 3, 2021

it seems it is running only for master...

Then we should change it to run on both master/release

I remember I saw it elsewhere...

Pylint should include it with this rule: https://pycodequ.al/docs/pylint-messages/w0703-broad-except.html

@Borda
Copy link
Collaborator Author

Borda commented Jan 3, 2021

Then we should change it to run on both master/release

can you send a PR with such config, because codefactor does not have any setting, all are taken from repo YAML files

Pylint should include it with this rule: https://pycodequ.al/docs/pylint-messages/w0703-broad-except.html

We are using flake8 but this shall be included too

@Borda
Copy link
Collaborator Author

Borda commented Jan 4, 2021

@tchaton @SeanNaren mind review? =)

@carmocca carmocca closed this Jan 4, 2021
auto-merge was automatically disabled January 4, 2021 08:04

Pull request was closed

@Borda Borda reopened this Jan 4, 2021
@Borda Borda merged commit 9575835 into release/1.2-dev Jan 4, 2021
@Borda Borda deleted the refactor/exceptions branch January 4, 2021 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready PRs ready to be merged refactor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants