Closed b5 closed 3 years ago
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
:memo: Please visit https://cla.developers.google.com/ to sign.
Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!
and we'll verify it.
âšī¸ Googlers: Go here for more info.
@googlebot I signed it!
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
:memo: Please visit https://cla.developers.google.com/ to sign.
Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!
and we'll verify it.
âšī¸ Googlers: Go here for more info.
@b5: Thanks for your quick reaction! I have duration()
and time()
as "constructors" and use parse_{time,duration}()
as parsing function. I think having the "class-name" as constructors is a matter of least-surprise and the the name for the parsing functions are obvious. However, I see that this would crush your current API...
Is there anything I can do to speed this up?
@alandonovan: I'm supporting this PR and will close mine once this one is merged.
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
:memo: Please visit https://cla.developers.google.com/ to sign.
Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!
and we'll verify it.
âšī¸ Googlers: Go here for more info.
@b5 any news on this one? I'm eagerly waiting for time support! ;-P
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
:memo: Please visit https://cla.developers.google.com/ to sign.
Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!
and we'll verify it.
âšī¸ Googlers: Go here for more info.
hi all,
I still have a little more cleanup to do here, but wanted to get this set of changes up for review. I've made a bunch of adjustments to the Starlark API based on review feedback.
@srebhan, what do you think the signature for the time.time
constructor should be? I've currently got it wrapping golang's time.Date
, but could also see removing time.from_timestamp
and using that instead.
@b5 I had time([year[, month[, day[, hour[, minute[, second[, nanosecond[, location]]]]]]]])
to mimic the python API a bit, but I think a kwargs
approach also works. Whatever you prefer to be honest! :-)
@b5 thanks for all your work! Walked through the code once again, but IMO we are very close. However I've no say here. ;-)
@srebhan I know you'd like to get this merged sooner rather than later, and I've got a number of other projects on the go at the moment. I've invited you as a collaborator on our fork, would you mind taking over this branch & making the necessary changes to get this up to speed? If not I'll get these changes from review made, but I can't guarantee they'll be done quickly.
@b5 I talked to my management and they were clear that my mandate only covers contributions to this repository. :-( So I think getting permission to contribute to your repo will take more time than it saves... This being said, I'm allowed to make suggestion in the comments and you can accept them. Would this be an option? Alternatively, we might ask @adonovan what is absolutely necessary to get this merged and then I will fix the remaining issues with an additional PR!?
ah that makes sense. I'll do what I can to make changes from review as soon as I can. Given that we can't directly collaborate, I think it'll be easier to just add another hour or two to clean this up & see if we can't get it merged. I'll try to get find that hour in the next day or two
Thanks a bunch!
Hi @b5, @srebhan asked me to help you on this, please check my contribution to it hoping it is useful https://github.com/qri-io/starlark-go/pull/1
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.
. If the bot doesn't comment, it means it doesn't think anything has changed.
âšī¸ Googlers: Go here for more info.
@googlebot I fixed it.
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.
We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent.
in this pull request.
Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla
label to yes
(if enabled on your project).
âšī¸ Googlers: Go here for more info.
@googlebot I consent.
@alandonovan I think I went through all your remarks but the one about the doc.go
, could you please clarify a little bit more what you have in mind? If you could check the rest also, it would be great. Thank you in advance đ
@alandonovan What do you suggest for lib/time/doc.go
? Maybe you would rather prefer that we remove it?
I'd vote to remove it. This PR shouldn't introduce a change to documentation conventions. It originated from a different codebase, and we can still maintain this form of documentation within that codebase
@b5 I've just removed it
FYI I renamed the package to starlarktime
to remain consistent with other modules like the json module
Sorry for radio silence: I'm on vacation at the moment but will attend to this PR fully on Monday.
On Sat, 20 Feb 2021 at 09:32, Nicolas Filotto notifications@github.com wrote:
FYI I renamed the package to starlarktime to remain consistent with other modules like the json module
â You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/google/starlark-go/pull/327#issuecomment-782690428, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABLFMP37RNWFAHJWNEUPBBDS77BWPANCNFSM4UNXYUDQ .
@alandonovan Remarks applied, please check again
Any update on this? :)
@b5 I cannot resolve conversations, could you please check if it is possible to give me the necessary rights?
@adonovan @alandonovan We really need this feature for https://github.com/influxdata/telegraf, so it is important for us to finish it and have it in Startlark. Can you still do the review or shall I ask someone else? and if so, who is the new project lead now please? (cc @ssoroka @srebhan)
@adonovan Thx for the feedbacks, it is hard to follow it properly since I have no way to resolve the discussions but I think that I addressed your feedbacks, could you please check it again?
quick think you to everyone who got this off the ground! @srebhan for motivating this in the first place, and @essobedo for putting in a huge amount of work to implement @adonovan's careful review đ
@b5 I also want to thank you for your continuous effort in bringing this forward. I know it's not easy to find the time. Special thanks to @essobedo for investing so much and pushing this over the finishing line!
@adonovan Thanks for the impressive review, can you still somehow merge ? or do you at least know who can merge?
Signed-off-by: b5 sparkle_pony_2000@qri.io
A few notes:
lib/time
based on @alandonovan's feedback from #326doc.go
into other formats. I can remove this to conform to other packages, but if this info can stay & be maintained, I'd be deeply appreciated. Keeping documentation to the code itself has made our lives much easier.