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

TimeSpan Literals and support for TimeSpan objects in Cmdlets such as Start-Sleep #246

Closed
wants to merge 12 commits into from

Conversation

schuelermine
Copy link

@schuelermine schuelermine commented Apr 13, 2020

RFC corresponding to PowerShell/PowerShell#12305

@msftclas
Copy link

msftclas commented Apr 13, 2020

CLA assistant check
All CLA requirements met.

@schuelermine schuelermine changed the title TimeSpan Literals and support for TimeSpan objects in Cmdlets such as Start-Sleep TimeSpan Literals Apr 20, 2020
@schuelermine schuelermine changed the title TimeSpan Literals TimeSpan Literals and support for TimeSpan objects in Cmdlets such as Start-Sleep Apr 20, 2020
@bpayette
Copy link

bpayette commented Feb 5, 2021

Please note that this is a breaking change since 15sec is a valid function name:

PS[1] (41) > function 15sec { "Hello" }
PS[1] (42) > 15sec
Hello

@joeyaiello
Copy link
Contributor

Given the comment I left in PowerShell/PowerShell#12305, it'd be awesome to get a couple updates to this RFC to reflect some of what we proposed for moving forward.

At that point, we can add our new Experimental - Approved label to this PR.

@schuelermine
Copy link
Author

@joeyaiello
Sorry for not responding any sooner.
Thank you for reviewing this proposal. I’ve read your comment and largely agree.
However, I need to apologize — I’m not very familiar with the RFC developement workflow and as such am unsure as to what’s expected of me; you say that “At this point, we'd be happy to see an experimental feature implementation submitted as a code PR.”, and I’m unsure if I’m required to provide this PR. I’d be happy to do so, but I’m unfortunately not a very experienced programmer and have little to no knowledge of C#.

@schuelermine
Copy link
Author

I’m going to update the RFC to reflect the proposed changed soon. Right now I’m somewhat busy, but I’ll get to it in anywhere from 2 hours to 2 days.

@joeyaiello
Copy link
Contributor

No worries at all! Thanks for getting back to us so quickly.

Basically, we're just saying that it's fine to commit the code as you're proposing it, but that it needs to be wrapped as an experimental feature so that we can learn more about how the feature's breaking changes impact real-world scenarios, and then decide further along in the process whether or not the functionality should remain and be on by default in future stable versions of PowerShell (upon which it would no longer be an experimental feature).

If you have any other questions, don't hesitate to ask!

@vexx32
Copy link
Contributor

vexx32 commented May 6, 2021

@schuelermine I won't lie, getting something like this built in is a chunk of work at some of the deepest levels of the parser and tokenizer. Probably the nearest thing to this proposal is some of my own PRs, some years ago -- probably this one is the best reference to look at: PowerShell/PowerShell#7901

If you're keen on doing the code I'm more than willing to offer help and pointers as best I can. It will likely be a somewhat lengthy process to get the code sorted and get through review.

If you'd rather leave it open for others to sort out the actual implementation that's OK as well, but there are no guarantees that the pwsh team or other community members will necessarily pick it up.

@schuelermine
Copy link
Author

OK, I’m going to update the PR today. Sorry for exceeding the maximum delay I set out…

@schuelermine
Copy link
Author

OK, I think this is done.

@schuelermine
Copy link
Author

Still contains one thing I’m unsure about, would like some feedback on that

@vexx32
Copy link
Contributor

vexx32 commented May 9, 2021

Still contains one thing I’m unsure about, would like some feedback on that

Re: the bit you weren't sure about - no auxiliary parameter should be needed, the parameter binding should be able to resolve the correct parameter based on the argument type.

@schuelermine
Copy link
Author

Oh right, we still need to specify that this is an experimental feature flag (initially). I’ll do that in a bit.

@joeyaiello
Copy link
Contributor

Looks great! Thanks for updating the RFC, and per @PowerShell/powershell-committee, feel free to move forward on the experimental implementation in PowerShell/PowerShell.

@joeyaiello joeyaiello added Experimental - Approved This RFC has been approved for a code submission as an experimental feature in PS/PS and removed Review - Waiting on Author labels May 18, 2021
Anselm Schüler added 2 commits May 18, 2021 21:52
This addresses a review comment by @SteveL-MSFT
(PowerShell#246 (comment))

modified:   2-Draft-Accepted/RFCNNNN-TimeSpan-Literals.md
@schuelermine
Copy link
Author

Hey, sorry, I haven’t actually started working on the feature yet, and it’s gonna be slow because I’m currently also working on another non-trivial project (where I’m also a volunteer and behind), but I’m gonna try to be more productive from now on.
(I’ll do stuff on schuelermine/PowerShell.)

@schuelermine
Copy link
Author

On another note, is the intent that this PR stays open until the feature is done? As I understood it, this repo’s for suggestions, so I was originally expecting this to be merged if the proposal is accepted. Doesn’t really matter, but I’d still like to know how this is organized.

@schuelermine
Copy link
Author

Hi, I just wanted to give an update—I haven’t done anything. I was recently approached to work on another project which has been taking up my time, but I will be getting to trying to make an implementation soon.

@joeyaiello
Copy link
Contributor

@schuelermine : thanks for the updates! The way our current process works, this would remain open until the final code is merged as non-experimental (which should only happen after the feature exists as an experimental feature in preview builds for some length of time).

So it's currently "experimentally approved" for you to start code work (or anyone else that wants to pick it up).

@joeyaiello joeyaiello added WG-Language parser, language semantics WG-Cmdlets general cmdlet issues labels Jul 27, 2021
@JamesWTruher
Copy link
Member

@PowerShell/powershell-committee reviewed
Timespan is supported via the new -Duration parameter, but because of the breaking nature of the new suffixes, we don't believe this should move forward.

@JamesWTruher JamesWTruher added Committee-Rejected The RFC was not accepted and removed Experimental - Approved This RFC has been approved for a code submission as an experimental feature in PS/PS labels Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Committee-Rejected The RFC was not accepted WG-Cmdlets general cmdlet issues WG-Language parser, language semantics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants