Skip to content

[github] Use Option<&str> instead of Option<DateTime> for `list_workf…#46

Closed
chantra wants to merge 1 commit into
oxidecomputer:mainfrom
chantra:datetime_in_workflow_list
Closed

[github] Use Option<&str> instead of Option<DateTime> for `list_workf…#46
chantra wants to merge 1 commit into
oxidecomputer:mainfrom
chantra:datetime_in_workflow_list

Conversation

@chantra

@chantra chantra commented Jan 31, 2023

Copy link
Copy Markdown
Contributor

…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

chantra commented Jan 31, 2023

Copy link
Copy Markdown
Contributor Author

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

chantra commented Feb 6, 2023

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
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

chantra commented Feb 8, 2023

Copy link
Copy Markdown
Contributor Author

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

chantra commented Mar 14, 2023

Copy link
Copy Markdown
Contributor Author

@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

chantra commented Mar 17, 2023

Copy link
Copy Markdown
Contributor Author

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

@chantra

chantra commented Mar 29, 2023

Copy link
Copy Markdown
Contributor Author

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

@augustuswm

Copy link
Copy Markdown
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

chantra commented Mar 29, 2023

Copy link
Copy Markdown
Contributor Author

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

chantra commented Mar 29, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

chantra commented Apr 4, 2023

Copy link
Copy Markdown
Contributor Author

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

chantra commented Apr 4, 2023

Copy link
Copy Markdown
Contributor Author

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

chantra commented Apr 21, 2023

Copy link
Copy Markdown
Contributor Author

@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