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

Add libdef for openai-node v3.3.0 #4459

Merged
merged 3 commits into from
Jun 22, 2023
Merged

Add libdef for openai-node v3.3.0 #4459

merged 3 commits into from
Jun 22, 2023

Conversation

joshuanapoli
Copy link
Contributor

@joshuanapoli joshuanapoli commented Jun 19, 2023

Other notes:

  • The flow executable version range is split only because of depending on axios types, which are split. At the moment, I still need the older range :-/
  • I generated the libdef using dts-bundle-generator and flowgen from the original typescript. I had to change the Axios type names, since typescript and flow use different names for the same types.
  • The build failed, but I don't think that this PR caused the problem:

    Package tua-body-scroll does not exist on npm, is this meant to be an environment instead?

@Brianzchen
Copy link
Member

The build failed, but I don't think that this PR caused the problem:

Thanks, geez my bad it's meant to be this https://www.npmjs.com/package/tua-body-scroll-lock.

Will look into it tonight and get back to you

@Brianzchen
Copy link
Member

Also this is quite a humongous definition file. Do you think more tests would be helpful? Or is the part you've tested all you need for it to be considered secure?

@Brianzchen
Copy link
Member

Pull from main, only your tests should impact now

@joshuanapoli joshuanapoli changed the title [openai] Add libdef Add libdef for openai-node v3.3.0 Jun 20, 2023
@joshuanapoli
Copy link
Contributor Author

joshuanapoli commented Jun 20, 2023

Also this is quite a humongous definition file. Do you think more tests would be helpful? Or is the part you've tested all you need for it to be considered secure?

It is covering the most common use-case of the library, which is pretty simple. (It's also the only case that I'm using.)

Pull from main, only your tests should impact now

Ok, I'll add a test case covering another piece of the library and pull from main.

@joshuanapoli
Copy link
Contributor Author

@Brianzchen thanks for your review. This PR is updated now and has passing tests.

@Brianzchen
Copy link
Member

Thanks I’ll do some sanity check tonight 🤞

@Brianzchen
Copy link
Member

Looks fine overall. Just one question, did you want to make the def 3.x.x instead of 3.3.x? Would make it easier to maintain with future minor releases, it's generally the standard unless you believe minors might cause dramatic breaking changes or you really want to maintain them separately.

@joshuanapoli
Copy link
Contributor Author

Looks fine overall. Just one question, did you want to make the def 3.x.x instead of 3.3.x? Would make it easier to maintain with future minor releases, it's generally the standard unless you believe minors might cause dramatic breaking changes or you really want to maintain them separately.

Ok, that sounds good. I'll push a change to 3.x.x.

@joshuanapoli
Copy link
Contributor Author

@Brianzchen thanks; I pushed c2221f1 renaming with more general versioning v3.x.x.

@Brianzchen Brianzchen merged commit 38adf5c into flow-typed:main Jun 22, 2023
4 checks passed
@joshuanapoli joshuanapoli deleted the openai/3.3.x branch June 23, 2023 18:38
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.

None yet

2 participants