Skip to content
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

Add options for not doing Windows-specific rules, not truncating #45

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

flotwig
Copy link

@flotwig flotwig commented Jan 19, 2019

I wanted to use this in the context of a web application which saves files on a GitHub repo (flotwig/markdown-notebooks) so I didn't need or want the parts that applied only to Windows filenames. So, I added an option to disable the last 2 regexes that are applied that are Windows-specific.

I also added an option to disable truncation, just for the sake of having some more options. :)

I added unit tests for the new functionality along with a switch for test.js to just run the unit tests. All tests pass, and this doesn't change the behavior the exposed API or the internal sanitize API for existing users.

@joelmukuthu
Copy link
Collaborator

Hi @flotwig, thanks for picking this up. I think a nicer way to add this functionality is adding a target option (perhaps an object with unix and windows properties, as boolean flags), see the discussion in #11 and #9 for the motivation. Particularly, the naming of that option needs to reflect that one is sanitizing for a certain environment, and not necessarily the environment they're on. target is just one suggestion :)

@flotwig
Copy link
Author

flotwig commented Jan 19, 2019

That sounds good. So I make sure I get this right, the unix flag would basically only target the third regex for .. and ., since the other two are multi-platform?

@joelmukuthu
Copy link
Collaborator

Hmm, I'm actually not sure that . and .. are valid on windows. @parshap, you probably know more on this? :)

@joelmukuthu
Copy link
Collaborator

Also, @parshap, is the 255 max-length a thing on all platforms?

@jackycute
Copy link

jackycute commented Feb 24, 2019

I'm using macOS 10.13.6 and my volume filesystem is APFS, the longest filename length looks like 241, not 255.

I was wrong, it is 255 but includes the filename extension (means XXX.md the .md parts will be counted in).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants