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

switch to using format string in prim logging #161

Merged
merged 2 commits into from
Sep 1, 2022
Merged

Conversation

quaquel
Copy link
Owner

@quaquel quaquel commented Sep 1, 2022

The main purpose is to fix #155 but I used this as an excuse to modify other logging message to switch to using format strings as well

@quaquel quaquel added this to the 2.3.0 milestone Sep 1, 2022
@coveralls
Copy link

coveralls commented Sep 1, 2022

Coverage Status

Coverage remained the same at 79.996% when pulling 6195339 on prim_logging into 23a22d4 on master.

Copy link
Collaborator

@EwoutH EwoutH left a comment

Choose a reason for hiding this comment

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

Looks good, added a few nitpicks.

It looks Flynt didn't catch all the strings that could be replaced with f-strings in #113. I might check this manually again sometime.

Comment on lines 1048 to 1050
(
"{} dropped from analysis " "because only a single category"
).format(column)
f"{column} dropped from analysis " "because only a single category"
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think both the break in stings " " could be removed, as well as the extra set of parentheses.

Comment on lines 1165 to 1168
(
"box does not meet threshold criteria, "
"value is {}, returning dump box"
).format(box.mean)
f"value is {box.mean}, returning dump box"
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this also fit on a single line? If yes, same as above

Copy link
Owner Author

Choose a reason for hiding this comment

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

some lines could indeed be reduced, see the latest commit. I already ran black on the file as well.

@quaquel quaquel mentioned this pull request Sep 1, 2022
@quaquel quaquel merged commit 35bc3ef into master Sep 1, 2022
@EwoutH EwoutH deleted the prim_logging branch September 2, 2022 06:57
@quaquel quaquel restored the prim_logging branch September 2, 2022 18:02
@EwoutH EwoutH deleted the prim_logging branch September 26, 2022 14:10
@EwoutH EwoutH restored the prim_logging branch September 26, 2022 14:10
EwoutH pushed a commit that referenced this pull request Oct 6, 2022
The main purpose is to fix #155 but I used this as an excuse to modify other logging message to switch to using format strings as well
@EwoutH EwoutH modified the milestones: 2.3.0, 2.2.1 Oct 6, 2022
@EwoutH EwoutH deleted the prim_logging branch October 10, 2022 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

small bug in prim logger
3 participants