Skip to content

Conversation

@frangio
Copy link
Contributor

@frangio frangio commented Dec 16, 2020

Fixes #152
Fixes #95
Fixes #199
Resolves #248
Resolves #249

  • Deprecation message for unsafeAllowCustomTypes flag. Kept for when struct members are missing from previous versions.
  • Deal with files (network files or validations.json) generated by previous plugin versions, since they are missing some new data we need.
  • Better diagnostic messages: explain why two structs are not considered equal, i.e. what members were removed/inserted/etc.
  • More tests.

Should this be considered a breaking change? I've currently made it so that an existing unsafeAllowCustomTypes flag will be ignored (and I'm additionally planning to make it print a deprecation message). If we assume that users were using this flag with truly compatible types (as intended) this it not a breaking change.

@frangio frangio changed the title Automatic checking of storage compatibility of complex types Automatic checking of storage compatibility of structs and enums Dec 17, 2020
updated: T[],
{ canGrow }: { canGrow: boolean },
): StorageOperation<T>[] {
const ops = levenshtein(original, updated, (a, b) => matchStorageField(a, b, { canGrow: false }));
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't canGrow be true for the last item in the storage? Though I'm not sure if that can open the door to other kind of problems, and it's best to be conservative.

Copy link
Contributor Author

@frangio frangio Dec 17, 2020

Choose a reason for hiding this comment

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

Yeah technically the last item in storage can grow, I think it's safe and I'd love to support it. The issue is that levenshtein calls the match function without any indication that an element is the last one in the layout. We would have to work around that or change the levenshtein function somehow and I decided to go with the simpler conservative choice.

If people run into this limitation we will reconsider!

Copy link
Contributor

@Amxx Amxx left a comment

Choose a reason for hiding this comment

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

I don't have a broad enough view to spot any subtle issues / inconsistency in the logic ... but overall, everything looks good at a local level.

@frangio frangio merged commit 963a2c5 into master Jan 27, 2021
@frangio frangio deleted the extract-structs branch January 27, 2021 00:13
@davidlucid
Copy link

Thank you so much! @frangio

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

5 participants