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

timestamp flag #987

Merged
merged 15 commits into from
Dec 20, 2019
Merged

timestamp flag #987

merged 15 commits into from
Dec 20, 2019

Conversation

drov0
Copy link
Contributor

@drov0 drov0 commented Dec 9, 2019

Fixes #696

Changes

This is a simple flag to add the support for timestamps. At first this flag was using protobuf timestamps but I ended up editing it since I figured you would be more interested in something more general. But if you guys are interested I would love to make a protobuf timestamp as well. (saves a few lines on my app code)

Reviewer Guidelines

I'm not sure if this code works with env variables. I tried to take inspiration from int64_slice and string_slice since those were the only flag that didn't use the default set from golang's Flag

I am open to any changes you see fit.

flag_timestamp.go Outdated Show resolved Hide resolved
flag_timestamp.go Outdated Show resolved Hide resolved
flag_timestamp.go Outdated Show resolved Hide resolved
flag_timestamp.go Outdated Show resolved Hide resolved
flag_timestamp.go Outdated Show resolved Hide resolved
Copy link
Member

@asahasrabuddhe asahasrabuddhe left a comment

Choose a reason for hiding this comment

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

Please add test cases to test the new functionality. Also update the manual.md to reflect the new features.

@drov0
Copy link
Contributor Author

drov0 commented Dec 12, 2019

@asahasrabuddhe

update the manual.md to reflect the new features.

Which part of the manual.md ? The full example ? There isn't much details on specific flags on the manual, It seems like most of the doc is auto generated through GoDoc

@asahasrabuddhe
Copy link
Member

@asahasrabuddhe

update the manual.md to reflect the new features.

Which part of the manual.md ? The full example ? There isn't much details on specific flags on the manual, It seems like most of the doc is auto generated through GoDoc

What I meant was that you add an example of using the timestamp flag as I think this is a unique flag, and users would appreciate an example on how to use it in their application.

@asahasrabuddhe
Copy link
Member

@drov0 If I initialize a timestamp flag without any default layout value, what layout is considered for the flag? Do you think having a default out of the box value for the layout would help?

1 similar comment
@asahasrabuddhe
Copy link
Member

@drov0 If I initialize a timestamp flag without any default layout value, what layout is considered for the flag? Do you think having a default out of the box value for the layout would help?

@drov0
Copy link
Contributor Author

drov0 commented Dec 14, 2019

@asahasrabuddhe, okay for the manual, will add on monday. As for the layout It won't work if you don't set it, the layout is mandatory. I considered having a default value but since there are so many different layouts, I figured that people would almost always use a custom layout anyways. Kind of like time.Parse doesn't have a default layout.

@asahasrabuddhe
Copy link
Member

@asahasrabuddhe, okay for the manual, will add on monday. As for the layout It won't work if you don't set it, the layout is mandatory. I considered having a default value but since there are so many different layouts, I figured that people would almost always use a custom layout anyways. Kind of like time.Parse doesn't have a default layout.

Fair enough. I think once we have an example, we are good to go 👍

@drov0
Copy link
Contributor Author

drov0 commented Dec 16, 2019

@asahasrabuddhe I added examples at the end in the full example, is that ok or would you rather that I add a complete new section for it ?

@coilysiren coilysiren merged commit ae84df4 into urfave:master Dec 20, 2019
@keyoub
Copy link

keyoub commented Dec 23, 2019

@drov0 thanks for adding this feature.

@lynncyrin any chance we can get this into a minor release (2.0.1 perhaps)? Would love to use the timestamp flag in my application with the proper go.mod routine.

@coilysiren
Copy link
Member

I'll draft a release soon 👍

@coilysiren coilysiren mentioned this pull request Dec 24, 2019
@coilysiren
Copy link
Member

=> #1013

@coilysiren
Copy link
Member

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.

TimeFlag
5 participants