-
Notifications
You must be signed in to change notification settings - Fork 59
[github] Use Option<&str> instead of Option<DateTime> for `list_workf… #46
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
Conversation
Notes:
|
@augustuswm this is also another PR I am looking for feedback. I am hoping the upstream API spec will get fixed, but in any cases, it seems this type of field will require special casing. |
I think the idea behind the change makes sense, the The change though needs to be more specific. As-is it is changing the generator so that anytime a parameter named I will try to find some time to look at how to get this integrated |
yeah, agreed. I did not find a way to compare to the "parent" spec. |
…low_runs`'s `created` parameter. The issue is described in more details in github/rest-api-description#2088 For now, this change will treat the created parameter as an optional str, which it would probably do if the parameter had a custom format defined. Alternatively, one oculd try to implement a proper datetime rnage that follows the [Query for dates](https://docs.github.com/en/search-github/getting-started-with-searching-on-github/understanding-the-search-syntax#query-for-dates) format, but this is maybe more work than one would benefit by implementing the search formating on the user side.
413355d
to
a60f6a0
Compare
@augustuswm I updated this diff. Here is the current change:
I am not sure there is much better options to handle |
@augustuswm not sure if you got as chance to look at this update. |
@augustuswm any opinion or objections on the latest proposal? Thanks |
I'll look at this today. Main thing I need to ensure is that this change only affects the necessary operations. (i.e. it doesn't leak to other parameters named |
So currently it will only change for GitHub project… but possibly any created parameter of that spec. IIRC, there is a couple of references to created and they all have the same fault, but it is not impossible that a legit datetime appears in this spec in the future. |
Another possibility would be to patch the spec. This way, there is no need to mess around within the code to special case stuff. This is probably the simplest and decently easy? WDYT? |
I went the path of patching the spec file (and it is now updated on main). If that looks correct I'll close this. |
Thanks! That look correct. I will try to rebase and run program on top of main to confirm this is actually doing what is expected. The only follow-up I would have is to keep the patch in the source tree and try to apply it on spec update, just to make sure that the change does not get lost over time. |
ok, I tried my program on top of main and it works as expected. I have #66 as a follow-up to make sure that the patch get applied on subsequent spec update until github/rest-api-description#2088 get resolved. |
@augustuswm I am going to close this PR given that the spec change works. I think we should have something along #66 though to make sure this does not regress. WDYT? |
…low_runs
's
created` parameter.The issue is described in more details in github/rest-api-description#2088
For now, this change will treat the created parameter as an optional str, which it would probably do if the parameter had a custom format defined.
Alternatively, one oculd try to implement a proper datetime rnage that follows the Query for
dates format, but this is maybe more work than one would benefit by implementing the search formating on the user side.