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

DM-34590: --longlog requires an unnecessary argument #245

Merged
merged 3 commits into from Apr 29, 2022

Conversation

PaulPrice
Copy link
Contributor

@PaulPrice PaulPrice commented Apr 28, 2022

--longlog is supposed to be a switch, and not require an argument,
but it was requiring one due to the absence of nargs=0.

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

--longlog is supposed to be a switch, and not require an argument,
but it was requiring one due to the absence of nargs=0.
@PaulPrice PaulPrice requested a review from ktlim April 28, 2022 16:58
@timj
Copy link
Member

timj commented Apr 28, 2022

I am pondering whether we really want to be doing gen2 fixes at this point since we are going to be deleting it soon. I think I'd rather there wasn't a change log entry for it since advertising gen2 bug fixes when we are saying everywhere else that gen2 is going away imminently might be more confusing than helpful.

@codecov
Copy link

codecov bot commented Apr 28, 2022

Codecov Report

Merging #245 (f6f13eb) into main (590c34a) will increase coverage by 0.03%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main     #245      +/-   ##
==========================================
+ Coverage   72.14%   72.17%   +0.03%     
==========================================
  Files          60       60              
  Lines        6628     6628              
  Branches     1250     1250              
==========================================
+ Hits         4782     4784       +2     
+ Misses       1621     1620       -1     
+ Partials      225      224       -1     
Impacted Files Coverage Δ
python/lsst/pipe/base/argumentParser.py 2.89% <0.00%> (ø)
python/lsst/pipe/base/graph/_loadHelpers.py 84.37% <100.00%> (ø)
python/lsst/pipe/base/graph/graph.py 81.23% <0.00%> (ø)
tests/test_quantumGraph.py 95.58% <0.00%> (+0.73%) ⬆️

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 590c34a...f6f13eb. Read the comment docs.

@PaulPrice
Copy link
Contributor Author

PaulPrice commented Apr 28, 2022

@andy-slac The mypy failure appears to be related to DM-33493.

python/lsst/pipe/base/graph/_loadHelpers.py:254: error: Argument 1 to "UUID" has incompatible type "UUID"; expected "Optional[str]"
python/lsst/pipe/base/graph/_loadHelpers.py:254: error: Subclass of "UUID" and "str" cannot exist: would have incompatible method signatures

@timj
Copy link
Member

timj commented Apr 28, 2022

Which was #242 and did pass mypy at the time. I wonder if the mypy version changed.

@timj
Copy link
Member

timj commented Apr 28, 2022

mypy was 0.942 and is now 0.950. (conda does not seem to have 0.950 yet)

Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

Looks fine. We aren't currently planning to back port this to v23 and I can't promise it will be in v24 (although it might be).

self.add_argument("--longlog", action=LongLogAction, help="use a more verbose format for the logging")
self.add_argument(
"--longlog",
nargs=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'm guessing this broke in DM-19634 (#178).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have no expectations that this will survive past the next weekly.

We don't want to give people the impression that Gen2 is under
active development.
python/lsst/pipe/base/graph/_loadHelpers.py:254: error: Argument 1 to UUID has incompatible type UUID; expected Optional[str]
python/lsst/pipe/base/graph/_loadHelpers.py:254: error: Subclass of UUID and str cannot exist: would have incompatible method signatures

The declared type of 'nodes' didn't match how it was being used
(declared as iterable of UUID, but treated as if it might be
an iterable of strings or UUID). Fixed the declared types rather
than changing the behaviour.

Looks like this only started showing up in mypy 0.950.
@PaulPrice PaulPrice merged commit 703ef5c into main Apr 29, 2022
@PaulPrice PaulPrice deleted the tickets/DM-34590 branch April 29, 2022 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants