Skip to content

Conversation

jmjf
Copy link
Contributor

@jmjf jmjf commented Aug 11, 2024

Closes #294

  • add test for schema named FooBARBaz (fails)
  • revert changes in PR#224
  • add FooBARBaz to example schema

Outstanding:

  • What is -p really supposed to do for oas2tson and oas2json?
  • Should openapi-transformer-toolkit support generating path specs? (discussion in: feat: generate schema from paths OpenAPI prop #230)
    • If not, does -p have a place in oas2tson or oas2json?
  • How to generate valid JavaScript identifiers for paths, choice to use - as replacement for / in paths, etc.?
  • Should oas2tson be refactored to align better with other commands -- and should that be the first next step?
    • Is there a tool that takes JSON and converts it to JavaScript? e.g., { "foo": 1, "bar": "barbar" } becomes { foo: 1, bar: "barbar" } If so, oas2tson = oas2json + convert to .js may be easier. (I tried searching, but of course all the answers are "JSON.parse()".)

@simoneb
Copy link
Member

simoneb commented Aug 11, 2024

@ilteoood can you take a look at this, as you were part of the conversation in the PR that this reverts?

Copy link
Collaborator

@ilteoood ilteoood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The revert LGTM, but why package.json changed while the lock didn't?

@jmjf
Copy link
Contributor Author

jmjf commented Aug 11, 2024

Thanks for the feedback.

Added commits for:

  • output files, including FooBARBaz.*
  • package.lock.json

I was looking at the 224 as a reference and missed the fact that it included package-lock.json. (This is why I need code reviewers. :) )

@simoneb
Copy link
Member

simoneb commented Aug 11, 2024

@jmjf if all your PR is doing is reverting #224, then maybe we should do it via GitHub, which reverts the commits altogether. I wonder though, the reasons why #224 was created are invalid or is the problem still there and we just need to seek an alternative solution?

@jmjf
Copy link
Contributor Author

jmjf commented Aug 12, 2024

@simoneb I'm okay to revert whichever way you think makes the most sense. The advantage of accepting this PR is that it adds a test that shows that assumption fails. If you want to revert with GitHub, I have no problem making a new PR to add that test. Whatever works best is fine.

#224 makes an invalid assumption about naming (changing FooBARBaz to FooBarBaz won't break anything -- false). Valid OpenAPI specs that generated TSON before that PR fail to generate TSON due to that invalid assumption. Solving the problem isn't simple and may be something this tool wants to avoid (next paragraph). I believe 224 should be reverted so folks aren't stuck using 1.4.1 until there's a good answer to the problem.

I think #224 was trying to implement TSON path generation, similar to oas2json generating JSON Schema path specs. Due to the naming issues involved, the problem is complex. Discussion in #230 suggests this tool may not want to support that feature. That makes me think the issue of path generation and -p paths needs a bigger discussion.

So I created #305 to try to get the discussion out of PR threads and into one, more visible issue.

@simoneb
Copy link
Member

simoneb commented Aug 12, 2024

Thanks @jmjf , it all makes sense to me. Let's merge this PR and resume the conversation in #305

@simoneb simoneb merged commit 3577a0b into nearform:master Aug 12, 2024
3 checks passed
@github-actions github-actions bot mentioned this pull request Aug 12, 2024
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.

oas2tson assumes strict camelcase naming -- bug
3 participants