Skip to content

Conversation

@JonWolfeDrata
Copy link

@JonWolfeDrata JonWolfeDrata commented Apr 1, 2025

Update all model-to-xxxxx that seemed affected by the N^M reference checks.

Fixes #56

@sinclairzx81
Copy link
Owner

sinclairzx81 commented Apr 11, 2025

@JonWolfeDrata Hi, Sorry for the delay on this one (have been flat out across projects lately)

Update all model-to-xxxxx that seemed affected by the N^M reference checks.

The updates look good (and thanks for going across each library) but can you please just comment out the previous logic rather than remove it. As per #56

... it would be good if you could comment the previous algorithm (rather than deleting) and drop a note in there about it being N^M + URL link to this issue. If there are any problems down the road, it just gives me a bit of historical context as to what's changed for that bit of code.

The reason is there are things downstream of this project where the previous resolver logic may be significant (although it's difficult to know without decent testing infrastructure in this project, something I haven't the bandwidth to put together at this time).

I am keen on your optimizations though, but if issues do occur downstream, I would be inclined to put the old N^M logic behind a configuration option (and probably look to fix everything up at a later time).

Other than that, all things look ok. Would it be possible to just comment out the previous code?

@JonWolfeDrata
Copy link
Author

@sinclairzx81 Sorry, had a baby so wasn't at work. Updated the PR to comment out the old code and put a reference to the original issue

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.

N^M algorithm in ModelToArkType + Fix

2 participants