Fix parsing of OPAMFETCH (support quotes / proper POSIX shell syntax) #5492
Conversation
kit-ty-kate
force-pushed
the
opamfetch-proper-cmd
branch
from
7204d79 to
6ef073f
Compare
|
It works! Thanks for the fix 👍 |
kit-ty-kate
force-pushed
the
opamfetch-proper-cmd
branch
from
232999b to
cf2f658
Compare
kit-ty-kate
force-pushed
the
opamfetch-proper-cmd
branch
from
cf2f658 to
1e862b7
Compare
| @@ -260,6 +260,12 @@ module List : sig | |||
| val fold_left_map: ('s -> 'a -> ('s * 'b)) -> 's -> 'a list -> 's * 'b list | |||
| end | |||
|
|
|||
| module Char : sig | |||
|
|
|||
| val is_whitespace : char -> bool | |||
There was a problem hiding this comment.
What about rather following what is being proposed upstream. That way, in a few years, you can likely simply delete the code. In other words:
module Char : sig
include Stdlib.Char
module Ascii : sig
val is_white : char -> bool
end
endThere was a problem hiding this comment.
I knew about the PR but I didn't use the same interface at the time for several reasons:
- the two definitions are different (e.g.
\vand\fare not handled by the opam definition but are with the stdlib one) - it wasn't merged at the time and we have a dedicated module for compatibility functions (https://github.com/ocaml/opam/blob/master/src/core/opamCompat.ml) so it would've felt a bit weird to have a yet-to-be-part-of-the-stdlib function in it
- OpamStd isn't meant to be a mirror of the stdlib in terms of naming, OpamCompat is
Now that it has been merged i can think about it again although i personally feel like it would be better to wait for the release of 5.4 to change the function. In the meantime i'm happy to add a TODO comment to remind ourselves to change it when 5.4 is released.
kit-ty-kate
force-pushed
the
opamfetch-proper-cmd
branch
from
1e862b7 to
491fc0c
Compare
| (* *) | ||
| (**************************************************************************) | ||
|
|
||
| val of_string : string -> string list |
There was a problem hiding this comment.
The new module should be in its own commit.
| (* *) | ||
| (**************************************************************************) | ||
|
|
||
| (* NOTE: Inspired from @dbuenzli's astring library *) |
There was a problem hiding this comment.
Maybe it worth adding an url ?
| (* *) | ||
| (**************************************************************************) | ||
|
|
||
| val of_string : string -> string list |
There was a problem hiding this comment.
Is there a reason to add a new module instead of adding it in OpamStd?
Fixes #5490