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

altsrc: Parse durations from strings #641

Merged
merged 2 commits into from
Jan 28, 2018

Conversation

cbranch
Copy link
Contributor

@cbranch cbranch commented Jul 6, 2017

Fixes part of #599.
I don't have a good idea (or the requirement) for fixing the other datatypes in that issue; presumably it should show the same leniency with exact types when parsing from weakly-typed data sources.

@jszwedko
Copy link
Contributor

👍 This seems reasonable to me, but do you mind adding a couple of tests for this behavior?

cc/ @ChrisPRobinson since I think you still have the most context on altsrc

@ChrisPRobinson
Copy link
Contributor

ChrisPRobinson commented Jul 11, 2017

@cbranch 👍, seems good other than tests.

Reviewing my code I think what I should have done re #599 was just to add those types to the system.
Uint, UInt64, Int64? Then have similar impls for those. But that would be a larger change. I think I mapped the types that existed at the time, or I just missed this. Not a fan of the boiler plate that is required but I don't see another way. Sorry for the bugs/design flaws...

I also like what you have done for duration as well, accepting a string seems good there

@cbranch cbranch force-pushed the altsrc-parse-durations branch 2 times, most recently from d09ed35 to c265724 Compare October 26, 2017 11:16
@cbranch
Copy link
Contributor Author

cbranch commented Oct 26, 2017

Finally got back to this one!
I added a simple test and moved the code into its own function to reduce the code reuse.
Regarding Uint/Uint64/Int64, these are larger changes because it requires an implementation of ApplyInputSourceValue for each of the flags, plus adding the new types on InputSourceContext interface/MapInputSource implementation. Since the workaround is easy (just use int) I'll punt for now.

Copy link
Contributor

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cbranch !

One nitpick, but otherwise this looks good to me. Thanks for adding the tests. I agree that we can just focus on this type for now.

}

return 0, nil
}

func castDuration(name string, value interface{}) (time.Duration, error) {
otherValue, isType := value.(time.Duration)
if !isType {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, but can we do an early return like

if isType {
    return otherValue, nil
}

...

Just to remove some of the nesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, done.

Copy link
Contributor

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thanks @cbranch !

@jszwedko
Copy link
Contributor

jszwedko commented Nov 6, 2017

Just waiting for build to finish (the failure looked like it might have been a CI flake).

@jszwedko jszwedko merged commit d3ae77c into urfave:v2 Jan 28, 2018
@jszwedko
Copy link
Contributor

Fixed the build failure in #703. Apologies for the delay!

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