Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Comments

Close side panel

[BREAKING] Replace --split with --merge-sources and Move generated extensions into their own source files#95

Merged
liamnichols merged 9 commits intoCreateAPI:mainCreateAPI/CreateAPI:mainfrom
LePips:merge-sources-and-split-extensionsLePips/CreateAPI:merge-sources-and-split-extensionsCopy head branch name to clipboard
Aug 4, 2022
Merged

[BREAKING] Replace --split with --merge-sources and Move generated extensions into their own source files#95
liamnichols merged 9 commits intoCreateAPI:mainCreateAPI/CreateAPI:mainfrom
LePips:merge-sources-and-split-extensionsLePips/CreateAPI:merge-sources-and-split-extensionsCopy head branch name to clipboard

Conversation

@LePips
Copy link
Contributor

@LePips LePips commented Aug 4, 2022

What

  • Makes splitting files the default and makes a new --merge-sources flag
  • Creates separate extension files per purpose at the top level instead of merging them
  • Updates tests to reflect

Copy link
Member

@liamnichols liamnichols left a comment

Choose a reason for hiding this comment

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

Great work on this, thanks for picking it up @LePips!

What is the notification however for moving the extensions out of Entities.swift and Paths.swift when using --merge-sources? It makes sense when splitting, but this one looks a little odd:

Screenshot 2022-08-04 at 20 42 28

I think I would have expected these to stay inside the merged file?


Secondly, I just want to bring this to your attention: #47

I'm thinking that maybe we can just go all of the way and move --merge-sources into the configuration options instead? WDYT about that approach?

There is also another part of that ticket that is relevant to you, which is about how to go about describing the filename templates in the config too so any feedback is welcomed 👍

@LePips
Copy link
Contributor Author

LePips commented Aug 4, 2022

extension using --merge-sources

I was under the impression that they should be broken out no matter what from #60 and personally think they should be. The only use case I can think of for merging schema-generated objects/paths would be for reference or other developer-meta purposes. While devs might also move files around, they will still have to be responsible for moving these extensions as well (split or merged).

Essentially, it splits the concerns of each file: Entities.swift for generated objects, Paths.swift for generated paths, and then each extension file is their own helper file. Additionally, the (default) naming scheme sorts them nicely into each section. We won't have to think further about that until I finish the filename template work.

#47

I don't think that this simple change in logic should worry about that until a consensus is reached there. I'll leave my thoughts on the issue.

@liamnichols
Copy link
Member

I was under the impression that they should be broken out no matter wha

Doh, you are right.. I forgot that I even suggested that 😅 #60 (comment)

I don't think that this simple change in logic should worry about that until a consensus is reached there. I'll leave my thoughts on the issue.

Sure 👍


Let me just do some final tests of this change 👀 It's hard to review on GitHub as the diff freezes up my browser 😄

Copy link
Member

@liamnichols liamnichols left a comment

Choose a reason for hiding this comment

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

It looks like the old Tests/CreateAPITests/Expected/petstore-split directory is still here, can we delete it please 🙏

Sources/CreateAPI/Generate.swift Outdated Show resolved Hide resolved
Co-authored-by: Liam Nichols <liam.nichols.ln@gmail.com>
@liamnichols liamnichols changed the title [Breaking] Split as default and split extension files [BREAKING] Replace --split with --merge-sources and Move generated extensions into their own source files Aug 4, 2022
@liamnichols
Copy link
Member

Please can you revert the rest of the changes to Snapshots.swift 🙏 Keep in mind that this diff is gigantic and barely loads in the browser so it makes it hard to efficiently review things 🙇

Its not worth changing it now, but my suggestion if we were to do this again would be to submit a pull request that switches --split for --merge-sources, but updates all the test fixtures that didn't use --split to use --merge-sources (and vice versa).. This would have helped us to be completely sure that the only other changes in output were related to moving the extensions since the diff would have been readable. We could then update all the fixtures to stop using --merge-sources in a separate PR where the diff is less important 🙏

Thanks again though for working on this! Will merge it once the checks finish ✅

@liamnichols liamnichols merged commit 683ecf4 into CreateAPI:main Aug 4, 2022
@liamnichols
Copy link
Member

Thanks again @LePips for the great contribution 🚀

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.

Make --split (-s) the default behaviour

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.