🔧 MAINTAIN: Update pylint version (& fix new issues) #5087

chrisjsewell · 2021-08-18T19:05:04Z

Update pylint pinning to v2.9 and remove restrictions on astroid and pylint-django.

django-not-available -> django-not-configured
Fix new categories: consider-using-dict-items, consider-using-enumerate, too-many-function-args, consider-using-from-import, consider-using-max-builtin, arguments-renamed, consider-using-with, use-maxsplit-arg, use-a-generator, deprecated-decorator
extra disables required for no-member (pylint seems to be getting worse at identifying members, or at least more strict)
disable unsubscriptable-object for cif.values

Update pylint to v2.9 and remove restrictions on astroid and pylint-django. - `django-not-available` -> `django-not-configured` - Fix new `consider-using-dict-items`, `consider-using-enumerate`, `too-many-function-args`, `consider-using-from-import`, `consider-using-max-builtin`, `arguments-renamed` - extra disables required for `no-member` - disable `unsubscriptable-object` for cif.vaues closes aiidateam#4647

chrisjsewell · 2021-08-18T19:11:31Z

Also I removed useless-suppressions from the disabled list (and even tried putting it in an enable list) but pylint still does not seem to identify them. I imagine there is quite a few "dead" ones in the code base now

there was some issues in pylint about this, but they claimed to be closed with the latest pylint (i.e. this one)

codecov · 2021-08-18T19:27:03Z

Codecov Report

Merging #5087 (af4f54a) into develop (c5e0118) will increase coverage by 0.03%.
The diff coverage is 85.15%.

@@             Coverage Diff             @@
##           develop    #5087      +/-   ##
===========================================
+ Coverage    80.50%   80.52%   +0.03%     
===========================================
  Files          531      531              
  Lines        37020    37095      +75     
===========================================
+ Hits         29799    29867      +68     
- Misses        7221     7228       +7

Flag	Coverage Δ
django	`75.02% <84.34%> (+0.03%)`	⬆️
sqlalchemy	`73.97% <82.74%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/cmdline/commands/cmd_daemon.py	`70.88% <0.00%> (ø)`
aiida/cmdline/commands/cmd_data/cmd_show.py	`16.67% <0.00%> (ø)`
aiida/cmdline/commands/cmd_data/cmd_structure.py	`84.06% <0.00%> (ø)`
aiida/orm/nodes/data/array/bands.py	`76.00% <0.00%> (ø)`
aiida/orm/nodes/data/array/projection.py	`15.45% <0.00%> (ø)`
aiida/orm/querybuilder.py	`83.94% <ø> (ø)`
aiida/tools/data/orbital/realhydrogen.py	`70.28% <0.00%> (ø)`
aiida/tools/data/structure.py	`91.97% <ø> (ø)`
aiida/tools/dbimporters/baseclasses.py	`75.71% <0.00%> (-0.71%)`	⬇️
aiida/tools/dbimporters/plugins/icsd.py	`20.98% <0.00%> (-0.15%)`	⬇️
... and 58 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5e0118...af4f54a. Read the comment docs.

chrisjsewell · 2021-08-18T20:18:05Z

there's 100s of raise-missing-from errors, so I'm leaving that as disabled for now

chrisjsewell · 2021-08-18T20:43:27Z

Intend to merge this in the next day, as it will affect all other PRs

sphuber

Have a couple of questions about certain disables. Initially repeated them at each line that I though they applied, but stopped after a while because there are so many instances. If you agree that in certain places we should actually fix instead of disabling, please have a look at other instances as well

sphuber · 2021-08-18T20:38:19Z

aiida/cmdline/commands/cmd_archive.py

@@ -254,6 +254,7 @@ def migrate(input_file, output_file, force, in_place, archive_format, version, v

 class ExtrasImportCode(Enum):
    """Exit codes for the verdi command line."""
+    # pylint: disable=invalid-name


Won't this now disable this for the entire file? Or is it properly scoped to the enum?

yeh, I was hoping it would be scoped to the enum, or do you think not?

Not sure to be honest. You could give it a quick try by adding a global constant after the enum declaration which should then incur a warning from pylint if it is properly scoped.

Yep looks to work as intended, FYI scoping is detailed here: http://pylint.pycqa.org/en/latest/user_guide/message-control.html#block-disables

sphuber · 2021-08-18T20:38:53Z

aiida/cmdline/commands/cmd_daemon.py

@@ -158,7 +158,7 @@ def logshow():

    try:
        currenv = get_env_with_venv_bin()
-        process = subprocess.Popen(['tail', '-f', client.daemon_log_file], env=currenv)
+        process = subprocess.Popen(['tail', '-f', client.daemon_log_file], env=currenv)  # pylint: disable=consider-using-with


Instead of disabling, couldn't we actually not better use a with statement here?

It wasn't quickly apparent to me how to do this and keep the same behaviour. i.e. is the same?

currenv = get_env_with_venv_bin() with subprocess.Popen(['tail', '-f', client.daemon_log_file], env=currenv) as process: try: process.wait() except KeyboardInterrupt: process.kill()

probably not 🤷

same with the other disable=consider-using-with

Fair enough. I would be tempted to do:

currenv = get_env_with_venv_bin() try: with subprocess.Popen(['tail', '-f', client.daemon_log_file], env=currenv) as process: process.wait() except KeyboardInterrupt: process.kill()

But I am not sure the try-except is even necessary when using the context manager. Keyboard interrupt should just kill it and the context manager should clean it up properly. Anyway, indeed fine to keep this as is as it is not a trivial change

sphuber · 2021-08-18T20:38:59Z

aiida/cmdline/commands/cmd_data/cmd_show.py

@@ -220,7 +220,7 @@ def _show_xmgrace(exec_name, list_bands):
            'agr', setnumber_offset=current_band_number, color_number=(iband + 1 % MAX_NUM_AGR_COLORS)
        )
        # write a tempfile
-        tempf = tempfile.NamedTemporaryFile('w+b', suffix='.agr')
+        tempf = tempfile.NamedTemporaryFile('w+b', suffix='.agr')  # pylint: disable=consider-using-with


Idem: Instead of disabling, couldn't we actually not better use a with statement here?

as above, what code would you propose? (just a more complex code changes then I was willing to make, when going through the 100s of issue fixes 😅)

I see now. Didn't notice it was creating a list of handles and then processing them. What the hell 🤦‍♂️ OK, fine to leave this as is for now

sphuber · 2021-08-18T20:39:20Z

aiida/cmdline/commands/cmd_run.py

@@ -114,7 +114,7 @@ def run(scriptname, varargs, auto_group, auto_group_label_prefix, exclude, inclu

    try:
        # Here we use a standard open and not open, as exec will later fail if passed a unicode type string.
-        handle = open(scriptname, 'r')
+        handle = open(scriptname, 'r')  # pylint: disable=consider-using-with


Idem: instead of disabling, couldn't we actually not better use a with statement here?

as above; basically I would maybe open a separate issue to address seom of the disable=consider-using-with, because some of them feel like they need a bit of scrutiny to double check they won't break the current behaviour

Good idea to open an issue and leave all of these with disable statements for now

aiida/cmdline/commands/cmd_status.py

aiida/cmdline/params/types/identifier.py

aiida/cmdline/params/types/path.py

aiida/orm/nodes/data/data.py

aiida/orm/utils/loaders.py

aiida/tools/data/cif.py

aiida/cmdline/params/types/path.py

chrisjsewell · 2021-08-18T21:10:37Z

addressed your comments thanks @sphuber

chrisjsewell · 2021-08-18T21:32:22Z

aiida/schedulers/plugins/lsf.py

+    @classmethod
+    def validate_resources(cls, **kwargs):
+        """Validate the resources against the job resource class of this scheduler.
+
+        :param kwargs: dictionary of values to define the job resources
+        :return: attribute dictionary with the parsed parameters populated
+        :raises ValueError: if the resources are invalid or incomplete
+        """
+


Guessing @abstractclassmethod didn't actually work, since this was not picked up before? @sphuber I guess this should probably have some content

Yeah that is weird indeed. I guess the validation is already done in the constructor. We can just move that logic to the validate_resources. To be safe as to not break anything, you can always still call self.validate_resources(**kwargs) from the constructor, which is what the NodeNumberJobResource also does.

sphuber

Thanks @chrisjsewell . Would be good if you can still check the comment on the Enum and the scheduler job resource validation, but am already approving this

for more information, see https://pre-commit.ci

chrisjsewell · 2021-08-19T22:26:17Z

all done cheers

chrisjsewell requested a review from sphuber August 18, 2021 19:05

chrisjsewell added 3 commits August 18, 2021 21:41

enable consider-using-with

9ce29cc

enable use-maxsplit-arg

5b01efb

enable use-a-generator

0540ffd

chrisjsewell added 2 commits August 18, 2021 22:30

enable deprecated-decorator

9fba371

fix abstract setters

74c229c

chrisjsewell changed the title ~~🔧 MAINTAIN: Update pylint version~~ 🔧 MAINTAIN: Update pylint version (& fix new issues) Aug 18, 2021

sphuber reviewed Aug 18, 2021

View reviewed changes

chrisjsewell commented Aug 18, 2021

View reviewed changes

aiida/cmdline/params/types/path.py Outdated Show resolved Hide resolved

chrisjsewell added 3 commits August 18, 2021 23:03

Update aiida/cmdline/params/types/path.py

1489e09

move # pylint: disable=unsubscriptable-object

a97b9d2

Update data.py

9900527

chrisjsewell requested a review from sphuber August 18, 2021 21:09

fix pre-commit

f4cda2e

chrisjsewell commented Aug 18, 2021

View reviewed changes

chrisjsewell mentioned this pull request Aug 19, 2021

✨ NEW: Add orm.Entity.fields interface for QueryBuilder #5088

Closed

sphuber previously approved these changes Aug 19, 2021

View reviewed changes

chrisjsewell mentioned this pull request Aug 19, 2021

Remove pylint consider-using-with disables #5090

Closed

Update lsf.py

c58cf68

chrisjsewell dismissed sphuber’s stale review via c58cf68 August 19, 2021 22:17

[pre-commit.ci] auto fixes from pre-commit.com hooks

af4f54a

for more information, see https://pre-commit.ci

chrisjsewell merged commit 3b5c975 into aiidateam:develop Aug 19, 2021

chrisjsewell deleted the update-pylint branch August 19, 2021 22:47

chrisjsewell mentioned this pull request Mar 12, 2022

🔀 MERGE: Release v2.0.0b1 #5426

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🔧 MAINTAIN: Update pylint version (& fix new issues) #5087

🔧 MAINTAIN: Update pylint version (& fix new issues) #5087

chrisjsewell commented Aug 18, 2021 •

edited

Loading

chrisjsewell commented Aug 18, 2021 •

edited

Loading

codecov bot commented Aug 18, 2021 •

edited

Loading

chrisjsewell commented Aug 18, 2021 •

edited

Loading

chrisjsewell commented Aug 18, 2021

sphuber left a comment

sphuber Aug 18, 2021

chrisjsewell Aug 18, 2021 •

edited

Loading

sphuber Aug 19, 2021

chrisjsewell Aug 19, 2021

sphuber Aug 18, 2021

chrisjsewell Aug 18, 2021

sphuber Aug 19, 2021

sphuber Aug 18, 2021

chrisjsewell Aug 18, 2021 •

edited

Loading

sphuber Aug 19, 2021

sphuber Aug 18, 2021

chrisjsewell Aug 18, 2021 •

edited

Loading

sphuber Aug 19, 2021

chrisjsewell commented Aug 18, 2021

chrisjsewell Aug 18, 2021

sphuber Aug 19, 2021

chrisjsewell Aug 19, 2021

sphuber left a comment

chrisjsewell commented Aug 19, 2021

🔧 MAINTAIN: Update pylint version (& fix new issues) #5087

🔧 MAINTAIN: Update pylint version (& fix new issues) #5087

Conversation

chrisjsewell commented Aug 18, 2021 • edited Loading

chrisjsewell commented Aug 18, 2021 • edited Loading

codecov bot commented Aug 18, 2021 • edited Loading

Codecov Report

chrisjsewell commented Aug 18, 2021 • edited Loading

chrisjsewell commented Aug 18, 2021

sphuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjsewell Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjsewell Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjsewell Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjsewell commented Aug 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sphuber left a comment

Choose a reason for hiding this comment

chrisjsewell commented Aug 19, 2021

chrisjsewell commented Aug 18, 2021 •

edited

Loading

chrisjsewell commented Aug 18, 2021 •

edited

Loading

codecov bot commented Aug 18, 2021 •

edited

Loading

chrisjsewell commented Aug 18, 2021 •

edited

Loading

chrisjsewell Aug 18, 2021 •

edited

Loading

chrisjsewell Aug 18, 2021 •

edited

Loading

chrisjsewell Aug 18, 2021 •

edited

Loading