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

Allow to configure the base for Integer parsing #1462

Closed
3 tasks done
ccremer opened this issue Aug 23, 2022 · 6 comments · Fixed by #1464
Closed
3 tasks done

Allow to configure the base for Integer parsing #1462

ccremer opened this issue Aug 23, 2022 · 6 comments · Fixed by #1464
Labels
area/v2 relates to / is being considered for v2 status/triage maintainers still need to look into this

Comments

@ccremer
Copy link
Contributor

ccremer commented Aug 23, 2022

Checklist

  • Are you running the latest v2 release? The list of releases is here.
  • Did you check the manual for your release? The v2 manual is here.
  • Did you perform a search about this feature? Here's the Github guide about searching.

What problem does this solve?

We pass a year and month as CLI flags/envs (each have their own flag/env var).
Unfortunately, when passing a month with preceding 0 like 08 for August it gets attempted to be parsed as octal, because of `strconv.ParseInt(..., 0, ...) (see

valInt, err := strconv.ParseInt(val, 0, 64)
) it's set to autodetect (https://pkg.go.dev/strconv#ParseInt).
In cases where the flag or env var is set by another tool with preceding 0, this can lead to unexpected issues like vshn/appuio-odoo-adapter#57.

If we could configure which base the parser should use, it would solve our issue.
See playground here: https://go.dev/play/p/a8VllHp_Z6D

Solution description

Add a property to all Int flags to let us explicitly configure the base for our parsing, e.g. Base or ParseBase (name tbd) that is then used in ParseInt.
Since Go's default value for Integers are 0, it would match what is currently used in ParseInt, thus I don't think it causes a breaking change.

Describe alternatives you've considered

  • Configure the month flag as StringFlag and do the parsing explicitly within the Before/Action functions.

Other workarounds are all outside of urface/cli:

  • Combine the year and month flag into a Timestamp flag with format ("yyyy-mm"), this would be a breaking change for the user.
  • Force any external tool (or human user) that provide the value for the month to omit leading zeroes when invoking the CLI. Depending on the tool used this might not always be possible.
@ccremer ccremer added area/v2 relates to / is being considered for v2 status/triage maintainers still need to look into this labels Aug 23, 2022
@davidgubler
Copy link

davidgubler commented Aug 23, 2022

In my opinion the default should be base 10 parsing and only base 10 parsing, as that is what a user would expect. Having such magic enabled by default causes unexpected reliability issues. Note that in our case of parsing months '01' through to '07' parse correctly, as do '10', '11' and '12'; the issue in this case is very subtle as it only applies to '08' and '09'. This is exactly why this kind of magic should not be the default.

Now I realize that changing this default behavior is a breaking change, but I think that this is something worth addressing in a major version in order to solve this problem once and for all for future users.

@dearchap
Copy link
Contributor

@ccremer Have you looked at using the cli.TimestampFlag for this purpose, you could do something like

--time 2006-01-02T15:04:05Z

@ccremer
Copy link
Contributor Author

ccremer commented Aug 23, 2022

@dearchap , yes I'm aware, I've listed it as a workaround in the alternatives already.

@dearchap
Copy link
Contributor

@ccremer Would you like to submit a PR for this ? Theoretically you could have your own implementation of a different base using GenericFlag. Adding it to all Int Flag types is a lot of work for us right now.

@ccremer
Copy link
Contributor Author

ccremer commented Aug 24, 2022

@dearchap I can try. I might be able to find a bit of time this week.

@ccremer
Copy link
Contributor Author

ccremer commented Aug 25, 2022

@dearchap I've opened #1464 as a start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v2 relates to / is being considered for v2 status/triage maintainers still need to look into this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants