Skip to content

Comments

feat(core): path connection strategy#32

Merged
jgoux merged 7 commits intomainfrom
jgoux/feat-core-path-connection-strategy
Apr 4, 2024
Merged

feat(core): path connection strategy#32
jgoux merged 7 commits intomainfrom
jgoux/feat-core-path-connection-strategy

Conversation

@jgoux
Copy link
Contributor

@jgoux jgoux commented Mar 13, 2024

Implementation of a path connection strategy, described here: https://discord.com/channels/788353076129038346/914821651017908234/1217410551916003328

We now have a connectPath which holds the models created before the current model, following the same "path".

If in the current model, a parent is not specified, we look into the connectPath for a similar model name, if we find one we connect to it.

The connect option takes precedence over the path strategy, so if you still want to use the path while connecting, it will have to be manual for now (using the connect callback).

Fixes: S-2014

@jgoux jgoux requested review from avallete and justinvdm March 13, 2024 14:11
@avallete
Copy link
Contributor

avallete commented Mar 13, 2024

I'm not a fan of changing behaviour while doing a rewrite, now we'll have to figure out if ported tests are failing because of expected change, or because of some bugs introduced in the rewrite.

I would like to postpone this kind of work (like for #21) until we migrated and released the first version of the package with a 1-1 behaviour with our current one.

IMO we'll be better of:

  1. Porting current behaviour in new version
  2. Porting test coverage for it.
  3. Releasing the new package with 1-1 behaviour compatibility (so users can start to migrate)
  4. Fix bugs / add new features once we got it all in the same stack and we can actually review the impact of each change with better test coverage.

@jgoux
Copy link
Contributor Author

jgoux commented Mar 13, 2024

I don't mind that we wait to merge then! 👍

@jgoux jgoux marked this pull request as draft March 13, 2024 14:23
@jgoux jgoux removed request for avallete and justinvdm March 13, 2024 14:23
@justinvdm
Copy link
Contributor

justinvdm commented Mar 13, 2024

I'm not a fan of changing behaviour while doing a rewrite, now we'll have to figure out if ported tests are failing because of expected change, or because of some bugs introduced in the rewrite.

I don't feel very strongly about it, but I wonder if there's merit to releasing all the breaking changes together. When we release this repo, it will already be coming with some breaking changes:

  • workflow: generate command -> introspect + generate commands
  • client now needing to be instantiated manually and passed to createSeedScript
  • some (hopefully minor) changes in deterministic results as a result of the template refactoring and related improvements

With this in mind, it might be better to also release other breaking changes like this one with it, so that the users can adjust once to the changes rather than several times.

I don't mind which way we go, but wanted to add my 2c.

edit: Merging after porting over tests makes sense regardless though!

@linear
Copy link

linear bot commented Apr 4, 2024

@jgoux jgoux marked this pull request as ready for review April 4, 2024 11:49
@jgoux jgoux requested review from avallete and justinvdm April 4, 2024 12:51
@jgoux jgoux enabled auto-merge (squash) April 4, 2024 13:16
@jgoux jgoux merged commit ceea0a7 into main Apr 4, 2024
@jgoux jgoux deleted the jgoux/feat-core-path-connection-strategy branch April 4, 2024 13:18
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.

3 participants