Skip to content

switch to using format string in prim logging#161

Merged
quaquel merged 2 commits intomasterfrom
prim_logging
Sep 1, 2022
Merged

switch to using format string in prim logging#161
quaquel merged 2 commits intomasterfrom
prim_logging

Conversation

@quaquel
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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