-
-
Notifications
You must be signed in to change notification settings - Fork 56
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
Fix git clone on Windows #868
Conversation
@adamlamar Thank you for the fix! I have only one very superficial objection. Question mark plays well with wtf (and that is why some mental effort was spent on picking up a good and short question-like meme when creating a new unit test for #866). Anyone looking at the new filenames may experience some cognitive dissonance. They might think: - WTF? Why "wtf#"? Shouldn't it be "wtf?"? 😄 They will definitely have hard time figuring out the reason behind this counterintuitive usage of the hash symbol. There are two solutions:
And, BTW, nothing prevents us from implementing both options - then both "#" and "?" will be tested! @mgautierfr What do you think? Should we just merge this PR or it makes sense to pursue consistency that perhaps no one will care about? |
Hey @veloman-yunkan, I agree it would be better if the test case made more sense in semantic terms. |
Oh and I agree that we could write a test case for all of the characters that need encoding ( |
test/data/corner_cases/c.html
Outdated
@@ -0,0 +1 @@ | |||
c#.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment applies to the name of the file.
Holding on to my previous concern wrt semantic consistency, I have to note that C is not C#. Maybe rename this file to c_sharp.html?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed now 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost good to merge.
However, I don't think that we need all those three commits (each with their own 36KiB version of corner_cases.zim
) in the history. Please squash all commits into one with a commit message explaining why the change is more extensive than it could have been.
227d3fc
to
8af2fb2
Compare
Sure, I squashed and pushed. |
The question mark (?) is not a valid filename character on Windows. Changing to a the pound sign (#) so that this repository can still be cloned on Windows.
8af2fb2
to
59012c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bearing with me and my whims!
This changes
wtf?
towtf#
. The?
is not allowed in filenames or directories on Windows, but#
is. If I understand #775 correctly, the#
should be a reasonably good substitute since it has the same requirements as?
.The corner case zimfile was also updated accordingly and the test should pass.
Successful clone on Windows:
Fixes #867