Skip to content

Fix string formatting in "dummy handlers"#2361

Merged
bsipocz merged 4 commits into
astropy:mainfrom
lvalerom:fix-string-formatting-in-dummy-handler
Apr 15, 2022
Merged

Fix string formatting in "dummy handlers"#2361
bsipocz merged 4 commits into
astropy:mainfrom
lvalerom:fix-string-formatting-in-dummy-handler

Conversation

@lvalerom

@lvalerom lvalerom commented Apr 15, 2022

Copy link
Copy Markdown
Member

Some of the strings in the "dummy handlers" were not properly formatted, as seen below:

# Notice that the parenthesis here are incorrect
ValueError("Method '{}' is not invoked. (Invoked method \
           is '{}'.)").format(method, self._invokedMethod)

As discussed here this PR fixes the string formatting of these modules and changes said formatting to f-strings:

ValueError(f"Method '{method}' is not invoked. (Invoked method "
           f"is '{self._invokedMethod}').")

@codecov

codecov Bot commented Apr 15, 2022 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #2361 (80883cf) into main (82136f4) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2361   +/-   ##
=======================================
  Coverage   63.17%   63.17%           
=======================================
  Files         132      132           
  Lines       17187    17187           
=======================================
  Hits        10858    10858           
  Misses       6329     6329

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

@lvalerom lvalerom requested a review from bsipocz April 15, 2022 18:59
@bsipocz bsipocz added this to the v0.4.7 milestone Apr 15, 2022
bsipocz
bsipocz approved these changes Apr 15, 2022
View reviewed changes

@bsipocz bsipocz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we don't need the return values, otherwise it look all very nice.

Comment thread astroquery/esa/hubble/tests/dummy_tap_handler.py Outdated
(str(key), method_name))
return False
raise ValueError(f"Parameter '{key}' not found in method '{method_name}'")
return True

@bsipocz bsipocz Apr 15, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this return value have any effect? As far as I see there are no asserts for the tests, so unless an exception is raised anywhere the tests using check_call would pass.

@lvalerom lvalerom Apr 15, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I noticed that as well but I left them just in case. But since you brought it up I'll remove them. I double checked the code and they're never called expecting them to return anything.

@lvalerom lvalerom force-pushed the fix-string-formatting-in-dummy-handler branch from fba1021 to f7230b9 Compare April 15, 2022 21:00
@bsipocz bsipocz merged commit 1368992 into astropy:main Apr 15, 2022
@bsipocz

bsipocz commented Apr 15, 2022

Copy link
Copy Markdown
Member

Thanks @lvalerom!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bsipocz bsipocz bsipocz approved these changes

Assignees

No one assigned

Labels

esa.esa_hubble esa.iso esa.jwst esa.xmm_newton no-changelog-entry-needed

Projects

None yet

Milestone

v0.4.7

Development

Successfully merging this pull request may close these issues.

2 participants

@lvalerom @bsipocz