Fix root directory regex #16
Conversation
|
Now CooperHewitt, Chapbook, and TexGyrePagella fonts are probably broken. |
|
Thank you for great contribution. I'm happy to see that you try to fix this issue. I know some otf directories should stay as root. Shall I explain the description of this issue to you? The fonts are distributed with some formats such as zip and tar.gz. Especially, zip causes this issue sometimes because the original creator of fonts zipped fonts with various directory structures.
Unfortunately, It's difficult to know which case the zip file is before converting from casks to formulae. We are welcome to fix this issue by you but we cannot merge your request without improvements of Regards. |
|
Thanks for the nice explanation 👋 I have already changed If structure is like first case it will go upper directory (any directory other than otf folder) My commit fails because it also changes directory if there is something like |
|
Okay, the regular expression becomes a complex notation if you try to make it run correctly for this case. I prefer to use |
|
I don't know much about ruby but I have a really terrible idea. Make a #{prefix} which is a shell %x( command ) looking for zipinfo for downloaded zip file in the .cache folder and if there is only one folder inside the zip except for dotfiles and __MACOSX then it can echo ../ else nothing. 😝 My last commit covers just a little bit better than before but it is not going to be a perfect solution ever. |
|
Wow, you're cool. It's an interesting solution but, as you said, it is also a terrible idea. |
|
Am I doing it right? This is the result: All of them has similar output |
|
Sorry for the late reply. I'll check soon. Give me a day. |
|
Issue #15 does block to merging this contribution. |
|
@cenksoykan I'm sorry for late reply. I'm so happy to inform you that we can merge this request! |
|
No problem at all, thanks for your attention from day zero 👏 this is awesome I am so glad you also fixed GitHub Actions quickly as possible 🎉 Sorry, I couldn't reply for a while but I will inform you soon |
|
OK I have just checked upstream modifications and everything is same as we talked. What do you want to know? In terms of clarity, if you merge this request those 6 fonts can be installed: BUT unfortunately Due to the zip issue you mentioned, some fonts cannot be fixed without a different approach and their status doesn't change: Btw, have you ever considered cleaning formula directory? There are 274 fonts removed from homebrew-cask-fonts therefore some fonts will not be affected ever or some give 404 error |
Sounds great, thanks.
For this case, we have
Okay, this issue should be pinned to our project until we solve it. At this time, you don't need to care about it. Your work already has enough value to merge our project!
OMG! Thanks for noticing that. I'm going to add the cleanup script as soon as possible. |
…into bugfix/otf-dir
|
Alright, I guess ready to review |
| @@ -74,7 +74,7 @@ class CaskTransform < Parslet::Transform | |||
| left + " " + right | |||
| } | |||
| rule(:font => simple(:font)) { | |||
| "(share/\"fonts\").install #{replace_cask_version_properties(font.sub(/"(.*\/)/, '"../\1'))}" | |||
| "(share/\"fonts\").install #{replace_cask_version_properties(font.sub(/"((?:(?!(otf|ttf)\/)[[:blank:][:punct:]]?.)+\/)/i, '"../\1'))}" | |||
There was a problem hiding this comment.
Your changes seem that it is almost so nice, but this regular expression is difficult for us to read and do refactoring.
Could you tell me the meaning of the dot .after [[:blank:][:punct:]]?.
There was a problem hiding this comment.
Any character. Actually I don't remember why I wrote like that and what is different than
"((?:(?!(otf|ttf)\/).*)+\/) Maybe, first I wrote something
"((?:(?!(otf|ttf))[[:blank:][:punct:]]?.)+\/) and changed it little bit and forgot about...
You are totally right
Forget it for now, I will open a new PR because 🥁 I found the solution. I hope so... I suppose it will be easier.
I'm sorry for taking your time with this request
|
Closed by #17 😍 |
I have not tested it out yet, but any directory path that starts with otf should be stay as root
Examples:
Fixes #8, and resolves #6