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

Make --split (-s) the default behaviour #74

Closed
Tracked by #101
liamnichols opened this issue Aug 2, 2022 · 5 comments · Fixed by #95
Closed
Tracked by #101

Make --split (-s) the default behaviour #74

liamnichols opened this issue Aug 2, 2022 · 5 comments · Fixed by #95
Labels
breaking Results in a breaking change in behaviour enhancement New feature or request
Milestone

Comments

@liamnichols
Copy link
Member

Background

Description

Bundling everything into just Paths.swift and Entities.swift prevents the compiler from parallelising build tasks and results in bad compile times for larger projects.

Therefore bundling the source has little advantage compared to splitting into individual files (--split) so we should make this the default.

We're not ready to remove the option completely, likely we'll replace it with something like --no-split or --merge but we should put less emphasis on it.

The one time that I've found it useful is for predicting the output sources so that we can make efficient BuildToolPlugin's. But in the long run, I want to explore ways to break out the evaluation from the generation so that we could compute this ahead of time... Maybe..

@liamnichols liamnichols added the enhancement New feature or request label Aug 2, 2022
@liamnichols liamnichols added this to the 0.1.0 milestone Aug 2, 2022
@liamnichols liamnichols added the breaking Results in a breaking change in behaviour label Aug 2, 2022
@liamnichols
Copy link
Member Author

There are two approaches that we could take here:

Add inversion to split

This would keep the -s/--split flag but also introduce an inversion called --no-split.

If you ran the generator without specifying -s/--split then it would result in an error:

$ create-api generate schema.json
Error: Missing one of: '--split', '--no-split'
Help:  --split <split>  Split output into separate files

Pros: Forces the users who didn't previously split to make a conscious decisions on the behaviour that they want
Cons: One extra flag that needs to be provided

Deprecate split and introduce mergeSources

For people using --split, we'd log a warning that the property was deprecated (and now the default behaviour), but for those not using --split, it will result in a change in generator behaviour.

Pros: Makes splitting the default behaviour
Cons: Breaking change


I think that I'm leaning towards a two-step change:

  1. Add the --split inversion to force people to make a conscious decision in 0.1.0
  2. Deprecate --split and --no-split in favour of --merge-sources in a future release

The result here is that there are no unexpected surprises if you don't read the release notes. If you want to continue merging your sources, you see an error, append --no-split and continue about your day. If you were already using --split, it makes no difference.

Then in a future update, we introduce --merge-sources and deprecate --split/--no-split, log a warning for a release or two and then remove the options.

@liamnichols
Copy link
Member Author

Removing the 0.1 milestone since this will be a longer-term project. The first phase however is going into 0.1 (see PR above)

@liamnichols liamnichols removed this from the 0.1.0 milestone Aug 4, 2022
@LePips
Copy link
Contributor

LePips commented Aug 4, 2022

In my opinion, since the release state of the project isn't >= 0.1 yet and we are in this phase of fast iterative development, we can safely implement these breaking changes without much concern. We are converging on some foundational implementations and rightly so at the beginning of the project. Essentially, I think it's better to get things done earlier in this phase than deprecate and wait.

@kean
Copy link
Member

kean commented Aug 4, 2022

I agree with @LePips. It's in a pre-release state. You should be able to freely introduce breaking changes. The initial set of options was added without much consideration for how they all fit together or any long term vision. The main focus was on getting the generation working and covering as many specs as possible.

@liamnichols
Copy link
Member Author

Thanks @LePips, @kean.. Yeah that makes sense, I think I am being too cautious for our current situation 😄

I'll switch straight to --merge-sources in that case.. How's that flag sound to you? Even more aggressive maybe, WDYT if I make it a configuration option instead? isMergingSources or isGroupingSources?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Results in a breaking change in behaviour enhancement New feature or request
Projects
None yet
3 participants