Skip to content

[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

Closed

Conversation

chantra
Copy link
Contributor

@chantra chantra commented Jan 31, 2023

…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.

@chantra
Copy link
Contributor Author

chantra commented Jan 31, 2023

Notes:

  • the relevant changes are really only in the generator. The rest came by just rebuilding the project. I am happy to strip those changes out.
  • I only rebuilt the github project, and disabled sync-ing the spec from github as it would fail to build with a bunch of errors not directly related to this.

@chantra
Copy link
Contributor Author

chantra commented Feb 6, 2023

@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.

@augustuswm
Copy link
Contributor

I think the idea behind the change makes sense, the created component certainly should not have a format property.

The change though needs to be more specific. As-is it is changing the generator so that anytime a parameter named created is encountered it alters the output type. What this needs to do is to alter the specific component, and only in the case that we are generating a GitHub client.

I will try to find some time to look at how to get this integrated

@chantra
Copy link
Contributor Author

chantra commented Feb 8, 2023

The change though needs to be more specific. As-is it is changing the generator so that anytime a parameter named created is encountered it alters the output type. What this needs to do is to alter the specific component, and only in the case that we are generating a GitHub client.

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.
@chantra chantra force-pushed the datetime_in_workflow_list branch from 413355d to a60f6a0 Compare March 14, 2023 03:54
@chantra
Copy link
Contributor Author

chantra commented Mar 14, 2023

@augustuswm I updated this diff. Here is the current change:

  • I get proper_name to trickle down to ParameterDataExt::render_type in order to be able to only change the handling of the data type when within github context
  • removed the generated files
  • I rebased on top of main

I am not sure there is much better options to handle ParameterDataExt::render_type better than passing proper_name as context.

@chantra
Copy link
Contributor Author

chantra commented Mar 17, 2023

@augustuswm not sure if you got as chance to look at this update.

@chantra
Copy link
Contributor Author

chantra commented Mar 29, 2023

@augustuswm any opinion or objections on the latest proposal? Thanks

@augustuswm
Copy link
Contributor

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 created if there are any)

@chantra
Copy link
Contributor Author

chantra commented Mar 29, 2023

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.

@chantra
Copy link
Contributor Author

chantra commented Mar 29, 2023

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?

@augustuswm
Copy link
Contributor

I went the path of patching the spec file (and it is now updated on main). If that looks correct I'll close this.

@chantra
Copy link
Contributor Author

chantra commented Apr 4, 2023

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.

@chantra
Copy link
Contributor Author

chantra commented Apr 4, 2023

I went the path of patching the spec file (and it is now updated on main). If that looks correct I'll close this.

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.

@chantra
Copy link
Contributor Author

chantra commented Apr 21, 2023

@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?

@chantra chantra closed this Apr 21, 2023
@chantra chantra deleted the datetime_in_workflow_list branch April 21, 2023 16:56
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.

2 participants