FIX: remove cwd from mac font path search #12177
Conversation
|
@dsentinel Thanks for doing the legwork to track this down! |
There was a problem hiding this comment.
You could leave the comma still in place. It’s a good style to also add a comma to the last element of a multi-line list. It’s less error prone if an additional element is later on (if one forgets to add the comma, we have implicit string concatenation - possibly happened exactly here before). Also adding a new line does not modify the previous line, which keeps the git history slightly more clean.
Just noticed, it‘s even mentioned in pep8
When to Use Trailing Commas
|
Apparently you cannot "like" reviews so I'll do it in text re: @timhoffm's review :p |
Happy to help! Glad you came to the same conclusion. |
While including the cwd makes sense at first glance, it does not because a) the result is cached so the next time your cwd will be different but we will not find those files b) the time it takes to search all the files is causing prolems c) the other 2 platforms do not do this The comma was introduced to fix what looked like a bug (implicit string concatenation instead of adding the empty string to the list) in matplotlib#11963. Original code come in via 4799341 in 2011. closes matplotlib#12176
tacaswell
force-pushed
the
fix_mac_font_search
branch
from
3697975 to
07e2d54
Compare
|
comma re-instated and force-pushed. |
|
Something went wrong ... Please have a look at my logs. |
|
@Carreau here is the error from the log: Seems to be some issues with switching branches prior to backporting. |
|
Also, I just noticed in the log, it tried to trigger commands from the last comment. I guess it should try to ignore any thing in code blocks if possible. |
Yep, I know I should fix that as well. I'll probably add the restriction that the mention should at start of line. I've added logic to erase the repo and clone from scratch if FF does not work. It should be up in a minute or so, Like to retrigger the backport ? |
|
@meeseeksdev backport to v3.0.x |
|
Something went wrong ... Please have a look at my logs. |
|
@Carreau not sure I see any new commits on https://github.com/MeeseeksBox/MeeseeksDev? |
|
@tacaswell Thanks for the feedback. I'm not concerned critizing code, I just wanted to motivate the style choice. If I had seen the PEP8 reference earlier, my comment would have been shorter 😜. |
|
Sorry about the issues it was late locally and I had brainfart. I exceptionally gave myself ability to mention MrMeeseeks here to retry the backport and not annoy you. @meeseeksdev backport to v3.0.x |
|
Awww, sorry Carreau you do not seem to be allowed to do that, please ask a repository maintainer. |
Well apparently I did not:-) Let's re-try. @meeseeksdev backport to v3.0.x |
…177-on-v3.0.x Backport PR #12177 on branch v3.0.x (FIX: remove cwd from mac font path search)
While including the cwd makes sense at first glance, it does not
because
a) the result is cached so the next time your cwd will be
different but we will not find those files
b) the time it takes to search all the files is causing problems
c) the other 2 platforms do not do this
The comma was introduced to fix what looked like a bug (implicit
string concatenation instead of adding the empty string to the list)
in #11963.
Original code come in via 4799341 in
2011.
closes #12176