Replace optparse with argparse in suite_report.py #34

t00sa · 2024-08-12T19:42:23Z

The only script using optparse was suite_report.py. This has been updated to use argparse and the script has been linted and its compliance with flake8 improved.

A before and after comparison of the output from suite_report.py shows no change in behaviour:

vdi> python suite_report.py -S ~/cylc-run/vn13.6_remove_optparse/runN/ -L /var/tmp
svn://fcm1/monc.xm_svn/casim/trunk appears to be the trunk, nothing to do!
svn://fcm1/jules.xm_svn/main/trunk appears to be the trunk, nothing to do!
svn://fcm1/moci.xm_svn/main/trunk appears to be the trunk, nothing to do!
svn://fcm1/um.xm_svn/mule/trunk appears to be the trunk, nothing to do!
svn://fcm1/utils.xm_svn/shumlib/trunk appears to be the trunk, nothing to do!
svn://fcm1/socrates.xm_svn/main/trunk appears to be the trunk, nothing to do!
svn://fcm1/ukca.xm_svn/main/trunk appears to be the trunk, nothing to do!
svn://fcm1/um.xm_svn/aux/trunk appears to be the trunk, nothing to do!
svn://fcm1/um.xm_svn/meta/trunk appears to be the trunk, nothing to do!
vdi> diff /var/tmp/trac.log ~/cylc-run/vn13.6_remove_optparse/runN/trac.log 
7c7
<  || Report Generated: || 2024/08/12 20:34:02 || 
---
>  || Report Generated: || 2024/08/12 13:53:01 ||
vdi>

See also: UM ticket 7722

Remove the deprecated optparse module and replace it with an similar implementation using argparse.

Find and fix various problems reported by pylint.

Fixes everything but a single case where the options are either an E129 warning about having the same indentation as the next line or an E127 warning about over-indentation - and there doesn't seem to be a format that satisfies the checker!

Enable command line argument completion for the two directory options if/when argcomplete is available.

To keep the linter happy, most of the format() calls with f-strings. This supposedly offers better performance and is more in keeping with python 3.

ericaneininger

A couple of comments/queries in-line.

suite_report.py

ericaneininger · 2024-09-03T15:33:43Z

suite_report.py

@@ -1274,7 +1281,7 @@ def write_lfric_testing_message(num_interactions):

        if num_interactions > 0:
            if num_interactions > 1:
-                message += ["There were {} projects ".format(num_interactions)]
+                message += [f"There were {num_interactions} projects "]


Appending a list of length one looks odd to me. Could use message.append(<msg>)
Having said that this structure is also used in the method create_approval_table(). Perhaps a pickiness too far!

I suspect there was a conscious decision not to use in-place additions rather than append() to be consistent with the sections that add multiple lines to the messages list. I'm happy to leave it be for now.

I've got wider refactoring changes that replace the list with an io.StringIO instance to make it possible to use print to construct an output buffer.

I suspect, you suspect wrongly....
I suspect it's a hold over from shell programming days and then when it threw an error the brackets to make it a list of one element were added to make the error go away....

But the ancients who wrote these hieroglyphs are long gone and we'll never be able to know for sure...

ericaneininger · 2024-09-03T15:35:57Z

suite_report.py

@@ -1712,13 +1724,15 @@ def gen_table_element(text_list, link, bold=False):
            if "working copy changes" in proj_dict:
                if proj_dict["working copy changes"]:
                    wc_text = "YES"
+                    # pylint: disable=consider-using-f-string


Having made so many the conversion to f-string format statements, there just 3 older style .format()s left. Why these?

Good point. It feels like there should be a more elegant way to format these string, but it's not immediately clear to me what it is. Perhaps they should be a property of the instance?

I'll take a look at the other pylint fstring exclusions and see if I can improve those too.

Happy to accept most of my comments will be addressed in the refactor ticket.

Would this be a good one to us os.path.join on instead of creating a string manually ?

I would always prefer to see os.path.join() rather than a string with /s in. It's then platform safe should someone try to run on Windows.

r-sharp

Hmmm, this looks like an also ticket....
Lets reconfigure it to use "argparse" and also do quite a lot of linting improvements...

Should it be 2 PRs 😃

Just looking at that - there were some "things were going to get back to later..." like the verbosity level which I'm amused to see are still untouched. Any chance they could be considered in the "major refactor" you mention ?

r-sharp · 2024-10-16T10:31:55Z

suite_report.py

-   UM repository. When making changes, please ensure the changes are made in
-   the UM repository or they will be lost during the release process when the UM
-   copy is copied over.
+   UM repository. When making changes, please ensure the changes are


This might seem 'pedantic' - but this comment is no longer true is it ?
The file has been moved from the UM repos, to the SimSys_Scripts one, and hopefully removed from all the repos it was in before. (this possibly needs checking)

So rather than change the line length (which I assume was done by black or similar, hence no-one noticing it was out of date) Could we remove this comment completely ?

r-sharp · 2024-10-16T10:34:23Z

suite_report.py

 """

+# pylint: disable=too-many-lines


I notice that quite a few directives for pylint have been added.
Should we either - have them all in one block at the top ?
Or - Always enable and disable the ones we want around the specific block causing us issues.
Or - both...

r-sharp · 2024-10-16T10:38:28Z

suite_report.py

@@ -330,7 +344,13 @@ def _parse_string(
    return value


-class SuiteReport(object):
+# pylint: disable=too-many-instance-attributes


AS in comment further up - do these need "deactivating" at the relevant point ? e.g. the bottom of this class ?

r-sharp · 2024-10-16T10:49:51Z

suite_report.py

@@ -1274,7 +1281,7 @@ def write_lfric_testing_message(num_interactions):

        if num_interactions > 0:
            if num_interactions > 1:
-                message += ["There were {} projects ".format(num_interactions)]
+                message += [f"There were {num_interactions} projects "]


I suspect, you suspect wrongly....
I suspect it's a hold over from shell programming days and then when it threw an error the brackets to make it a list of one element were added to make the error go away....

But the ancients who wrote these hieroglyphs are long gone and we'll never be able to know for sure...

r-sharp · 2024-10-16T10:52:16Z

suite_report.py

@@ -1355,6 +1367,8 @@ def generate_task_table(
        if not empty and indicates how many tasks of the relevant types have
        been hidden"""

+        status_counts = status_counts or defaultdict(int)


How/Why is doing this here better than giving status_counts the default value of defaultdict(int) in the argument list - they seem to be identical to me....

r-sharp · 2024-10-16T11:00:53Z

suite_report.py

                except KeyError:
                    pass
            trac_log.append(
+                # pylint: disable=consider-using-f-string


hmmmmm, .... Yum !
What about breaking it into 2 lines...
bg_colour = BACKGROUND_COLOURS[self.primary_project.lower()]
trac_log.append(
f"{{{{{{#!div style='background : {bg_colour}'"

I appreciate it's just the same thing another way, but it should (hopefully) dispose of 2 linter directives...

r-sharp · 2024-10-16T11:03:03Z

suite_report.py

            trac_log.append(
-                " || Cylc-Review: || {0:s}/{1:s}/{2:s}/?suite={3:s} || ".format(
+                " || Cylc-Review: || {0:s}/{1:s}/{2:s}/?suite={3:s} || "


Another where it might be useful to split out into multiple 'steps'
use os.path.join() to join all the parts together into a string which you assign to "sodding_path", and then use an f-string with {sodding_path} in it.

r-sharp · 2024-10-16T11:12:25Z

suite_report.py

+        # on a JULES rose-stem suite as JULES has no 'need' of the two
+        # compare variables and to prevent the warning their absence
+        # would produce from occuring unnecessarily in JULES they have
+        # been added to rose-suite.conf for now


In general - are these not dead yet ? does the UM trac.log still report on compare wallclock and compare output tests ?

r-sharp · 2024-10-16T11:18:15Z

suite_report.py

@@ -2009,17 +2029,19 @@ def print_report(self):
                trac_log.append("")
                trac_log.append("-----")
                trac_log.append(" = WARNING !!! = ")
+                # pylint: disable=consider-using-f-string
                trac_log.append(
                    "This rose-stem suite included multiple "
                    + "branches in {0:d} projects:".format(
                        len(self.multi_branches.keys())
                    )


trac_log.append( "This rose-stem suite included multiple " + f"branches in {len(self.multi_branches.keys())}" + " projects:"

Might get rid of the linter directives - I /think/ the line isn't too long...

r-sharp · 2024-10-16T11:20:31Z

suite_report.py

                raise

+        # pylint: disable=broad-exception-caught


Is this needed here because it looks like it might have been an attempt to re-enable broad-exception-caught ?

t00sa added 3 commits August 12, 2024 17:59

Repalce optparse with argparse in suite_report.py

7b1ccde

Remove the deprecated optparse module and replace it with an similar implementation using argparse.

Fix linting problems in suite_report.py

2f676cb

Find and fix various problems reported by pylint.

Make suite_report.py flake8 compliant

a265ebd

Fixes everything but a single case where the options are either an E129 warning about having the same indentation as the next line or an E127 warning about over-indentation - and there doesn't seem to be a format that satisfies the checker!

t00sa linked an issue Aug 12, 2024 that may be closed by this pull request

Replace optparse with argparse in scripts #33

Open

t00sa added 2 commits August 13, 2024 10:54

Add argument completion to suite_report.py

555237d

Enable command line argument completion for the two directory options if/when argcomplete is available.

Refactor with f-strings in place of formats

8c468cf

To keep the linter happy, most of the format() calls with f-strings. This supposedly offers better performance and is more in keeping with python 3.

t00sa requested review from a team and ericaneininger and removed request for a team August 27, 2024 07:22

ericaneininger reviewed Sep 3, 2024

View reviewed changes

jennyhickson requested a review from r-sharp September 13, 2024 09:47

ericaneininger approved these changes Sep 13, 2024

View reviewed changes

r-sharp requested changes Oct 16, 2024

View reviewed changes

r-sharp assigned t00sa Oct 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace optparse with argparse in suite_report.py #34

Replace optparse with argparse in suite_report.py #34

t00sa commented Aug 12, 2024

ericaneininger left a comment

ericaneininger Sep 3, 2024

t00sa Sep 4, 2024

r-sharp Oct 16, 2024

ericaneininger Sep 3, 2024

t00sa Sep 4, 2024

ericaneininger Sep 13, 2024

r-sharp Oct 16, 2024

ericaneininger Nov 8, 2024

r-sharp left a comment •

edited

Loading

r-sharp Oct 16, 2024

r-sharp Oct 16, 2024

r-sharp Oct 16, 2024

r-sharp Oct 16, 2024

r-sharp Oct 16, 2024

r-sharp Oct 16, 2024

r-sharp Oct 16, 2024

r-sharp Oct 16, 2024

r-sharp Oct 16, 2024

r-sharp Oct 16, 2024

Replace optparse with argparse in suite_report.py #34

Are you sure you want to change the base?

Replace optparse with argparse in suite_report.py #34

Conversation

t00sa commented Aug 12, 2024

ericaneininger left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-sharp left a comment • 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

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

r-sharp left a comment •

edited

Loading