这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@RL-S
Copy link
Contributor

@RL-S RL-S commented Jul 12, 2022

Description

The gotta-patch-em-all-font-patcher!.sh assumes that it's being run from inside bin/scripts, by setting source and target directories according to the output of pwd.
That's not super obvious from the outside. When I tried running it, I didn't know and tried running it from the root dir. That didn't cause an error, but instead the script simply didn't find any fonts.

Now it sets the directories relative to the script directory, which allows running it from anywhere.

What does this Pull Request (PR) do?

Added one line plus comment to the script, to set the script directory (var sd). Then changed the two instances of ${PWD} to instead use ${sd}.

How should this be manually tested?

Run the script from inside or outside bin/scripts, see if it works. I'm currently running it from the root dir, and it's working fine so far.

The script assumes that it's run from inside bin/scripts, by setting source and target directories according to the output of pwd. It doesn't perform any checks whether it's run from the right directory, so it just doesn't find the fonts if the working directory is different.
Now it sets the directories relative to the script directory.
@RL-S
Copy link
Contributor Author

RL-S commented Jul 12, 2022

Dummy comment to invoke bot, according to contributing guidelines.

@Finii
Copy link
Collaborator

Finii commented Jul 12, 2022

That's not super obvious from the outside.

Well, it is ... somehow, when you read the comment in the top:

# for executing script to rebuild JUST the readmes:
# ./gotta-patch-em-all-font-patcher\!.sh "" info

You see, it assumes the script to be in the current dir . ;-D

It is there, but not very well documented. Also your change is a good improvement!
I also always cry out in frustration about the scripts there 😬
Thanks for the PR.

@RL-S
Copy link
Contributor Author

RL-S commented Jul 12, 2022

It's clear that it's done that way in the README, but not that it has to be done that way. And if I can save writing cd, by only forking a project, writing a commit description and engaging with other contributors, then that's clearly a win :D

@Finii
Copy link
Collaborator

Finii commented Jul 16, 2022

Approve ✔️

@RL-S
Copy link
Contributor Author

RL-S commented Jul 17, 2022

Anything still required from my side? Feel free to edit the commit/PR, I'm not passionate about the details.

@Finii
Copy link
Collaborator

Finii commented Jul 17, 2022

Anything still required from my side? Feel free to edit the commit/PR, I'm not passionate about the details.

Hmm, I was certain that there is a term in the contribution guidelines, that merges / pulls into master are only to be done by Ryan.
But in the current contributing.md I can not find it. And even not in older versions. Strange. That was the reason why I (almost) never merged anything into master, but it seems to be a misunderstanding?

@Finii Finii merged commit f18c02f into ryanoasis:master Jul 26, 2022
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.

2 participants