Skip to content
This repository has been archived by the owner on Jul 1, 2024. It is now read-only.

Show differences from the required form #224

Closed
wants to merge 2 commits into from

Conversation

jablko
Copy link
Contributor

@jablko jablko commented Nov 12, 2020

to help authors react to them.

Only shows the new differences, not any existing diffs. notRemove.map(({ path }) => newDiff[Number(path.slice(1))]) assumes the notRemove operations are in ascending path order (e.g. /1, /2, /10, etc.).

@jablko jablko marked this pull request as ready for review November 12, 2020 19:36
Copy link
Contributor

@elibarzilay elibarzilay left a comment

Choose a reason for hiding this comment

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

  1. I'm not sure about showing these: in the non-tsconfig cases, the "required form" is not as required. Perhaps it could work if there's a way to verbalize the ignored properties too. (Though it still needs to end up as a short sentence.)
  2. I'd like to keep these as one-liners, eg `...\`${JSON.stringify(...)}\`...` instead of the \`\`\`s.
  3. Needs tests.

@jablko
Copy link
Contributor Author

jablko commented Nov 13, 2020

  1. I'm not sure about showing these: in the non-tsconfig cases, the "required form" is not as required. Perhaps it could work if there's a way to verbalize the ignored properties too. (Though it still needs to end up as a short sentence.)

Agree there's room for improvement.

  1. I'd like to keep these as one-liners, eg `...\`${JSON.stringify(...)}\`...` instead of the \`\`\`s.

Done ✔️

  1. Needs tests.

I'll add a fixture when I catch a PR before it's merged 👍

@jablko
Copy link
Contributor Author

jablko commented Nov 25, 2020

Closing in favor of #235

@jablko jablko closed this Nov 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants