Skip to content

Fix option ED2K-port and similar#64

Closed
nagius wants to merge 1 commit into
ygrek:masterfrom
nagius:fix-issue-34
Closed

Fix option ED2K-port and similar#64
nagius wants to merge 1 commit into
ygrek:masterfrom
nagius:fix-issue-34

Conversation

@nagius

@nagius nagius commented Jun 26, 2021

Copy link
Copy Markdown

This PR fix the option parsing issue reported in #34.

It basically split the option name in two with the separator - to keep only the right part.
Fall back on previous way in case of error.

@nagius nagius mentioned this pull request Jun 26, 2021
@lulol

lulol commented Jul 2, 2021

Copy link
Copy Markdown

List.nth won't keep only the right part but only the nth element of the split string. This works because there are no simple options containing an '-'.

And String.split_on_char is only available since 4.04.0 so will fail compiling with older 3.x versions?

May be you could use instead something like this that will address both issues:

let name = String.sub oi.M.option_name (String.index oi.M.option_name '-' + 1) (String.length oi.M.option_name - 1 - String.index oi.M.option_name '-')

But don't ask me to make it less ugly because I'm clueless about OCaml.

@nagius

nagius commented Jul 11, 2021

Copy link
Copy Markdown
Author

Yes indeed, List.nth will keep de second element of the string, but there is only one - in every option names. The separator used in the option names is _, so that seems to be a reasonable shortcut.

No idea about the OCaml version, I compiled it with the last version available on Ubuntu 20.20 using docker (see #65). Do we want to keep compatibility with older OCaml ?

TBH, OCaml looks alien to me, I'm surprised I succeeded to compile this patch.

@ygrek

ygrek commented Jul 30, 2024

Copy link
Copy Markdown
Owner

thanks! fixed it in a little cleaner way in 911fa54

@ygrek ygrek closed this Jul 30, 2024
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.

3 participants