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

Extend flake8 linting to ert-directory and ert_tests #3203

Merged
merged 1 commit into from
Apr 7, 2022

Conversation

berland
Copy link
Contributor

@berland berland commented Apr 5, 2022

Issue
Resolves missing flake8 requirements for parts of the codebase

Approach
Add flake8 testing in Github Actions runner to include more directories, and fix related flake8 issues
in those directories.

Pre review checklist

  • Added appropriate release note label
  • PR title captures the intent of the changes, and is fitting for release notes.
  • Commit history is consistent and clean, in line with the contribution guidelines.

Adding labels helps the maintainers when writing release notes. This is the list of release note labels.

@berland berland added the release-notes:maintenance Automatically categorise as maintenance change in release notes label Apr 5, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2022

Codecov Report

Merging #3203 (805716d) into main (e9153bf) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #3203   +/-   ##
=======================================
  Coverage   65.60%   65.61%           
=======================================
  Files         614      614           
  Lines       48977    48983    +6     
  Branches     4407     4407           
=======================================
+ Hits        32133    32138    +5     
  Misses      15374    15374           
- Partials     1470     1471    +1     
Impacted Files Coverage Δ
ert/ensemble_evaluator/activerange.py 97.67% <ø> (ø)
libres/lib/res_util/block_fs.cpp 52.85% <0.00%> (-0.17%) ⬇️
ert_shared/main.py 86.47% <0.00%> (+0.40%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@berland berland self-assigned this Apr 5, 2022
@berland berland force-pushed the flake_ert branch 2 times, most recently from cf1178a to 15e7d19 Compare April 6, 2022 11:41
Copy link
Contributor

@kvashchuka kvashchuka left a comment

Choose a reason for hiding this comment

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

The PR looks good to me 👍
Good job!

f"\nexperiment_json: {json.dumps(experiment_json, indent=1)} \n\n"
f"ensemble_json: {json.dumps(ensemble_json, indent=1)}\n"
f" status_code: {resp.status_code}"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity - why do you have brackets () here? Why not just split the line using backslash?

Copy link
Contributor

Choose a reason for hiding this comment

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

also, imports in this file can be optimised - pylint picks up that pytest is not used and the order is

import io
import json
import uuid

import pandas as pd
from numpy.testing import assert_array_equal
from requests import Response

P.S. Sorry for putting the comment here, could not figure out how to attach it to imports when you haven't made changes there ..

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, we do not do pylint on tests, and this PR is about flake8, so I retrieve my comment about imports :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose round-bracket-string-concatenation because I found it a tad more readable here. But that might be a matter of taste.

Sorted imports (isort) and removed the superfluous import in this file. Bonus change..

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I already googled that brackets is a preferred way to split the line and not backslash

@berland berland enabled auto-merge (rebase) April 7, 2022 14:13
@berland berland merged commit 1a84fa9 into equinor:main Apr 7, 2022
@berland berland deleted the flake_ert branch June 15, 2022 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:maintenance Automatically categorise as maintenance change in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants