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

Filename.chop_suffix_opt #2125

Merged
merged 5 commits into from Nov 8, 2018

Conversation

Projects
None yet
4 participants
@alainfrisch
Copy link
Contributor

commented Oct 29, 2018

This supersedes #1858 and somehow addresses parts of MPR#7812. cc @whitequark

A new function is added:

val chop_suffix_opt: suffix:string -> string -> string option
(** [chop_suffix_opt ~suffix filename] removes the suffix from
    the [filename] if possible, or returns [None] if the
    filename does not end with the suffix.

The goal is to deprecate Filename.chop_suffix eventually. That function is dangerous: it only removes the final characters from the filename, without actually checking the suffix matches; and it raises only when the filename is "too short", which is rare enough to have the bugs fly under the radar for a long time.

Following the "modern" style in the stdlib, the new function returns an option instead of raising an exception in case of mismatch. This could avoid bugs such as MPR#1858.

The function uses a labelled argument to avoid the possible confusion between the two arguments. check_suffix/chop_suffix are rare cases where I always need to look up the documentation to know the ordering of arguments.

This PR does not rewrite the uses of chop_suffix in the core distribution to use the new function. This can be done as a follow-up PR.

Also, this PR does not mark Filename.chop_suffix as being deprecated; in the latest caml-devel meeting, it was mentioned that having one full major version of OCaml with the new functions before deprecating the "old" ones is considered to be the polite behavior towards the community.

@whitequark

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

Thank you!

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

Anyone to review this ? Perhaps @nojb ?

@nojb

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2018

I will take a look.

@nojb nojb self-assigned this Nov 6, 2018

@nojb

nojb approved these changes Nov 6, 2018

Copy link
Contributor

left a comment

LGTM.

I wonder if we should document in the doc for check_suffix and chop_suffix_opt that under Windows we compare the extension modulo ASCII-case folding (which may not match the underlying OS behaviour in the case of non-ASCII characters, see https://blogs.msdn.microsoft.com/greggm/2005/09/21/comparing-file-names-in-native-code/).

Also, don't forget to update Changes with GPR number.

Show resolved Hide resolved stdlib/filename.mli Outdated
@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

Thanks, all comments addressed.

Btw, do you know why we don't use (pseudo)-case insensitive comparison under MacOS as well?

@nojb

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2018

Btw, do you know why we don't use (pseudo)-case insensitive comparison under MacOS as well?

No idea.

@alainfrisch alainfrisch merged commit a7a76fd into ocaml:trunk Nov 8, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@damiendoligez

This comment has been minimized.

Copy link
Member

commented Nov 16, 2018

MacOS supports both case-sensitive and case-insensitive file systems. And I wouldn't be surprised if Windows did too...

@nojb

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2018

Windows supports case-sensitive and case-insensitive directories: https://blogs.msdn.microsoft.com/commandline/2018/02/28/per-directory-case-sensitivity-and-wsl/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.