Skip to content

Conversation

@ryzokuken
Copy link
Contributor

The current direction for the assert diffs is rather peculiar.
While it’s not the “wrong” way to do it per-se, it’s definitely
inconsistent with the ecosystem and is rather unintuitive.

This PR aims to fix that by reversing the direction.

Fixes: #20897

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

/cc @BridgeAR @addaleax @targos

The current direction for the assert diffs is rather peculiar.
While it’s not the “wrong” way to do it per-se, it’s definitely
inconsistent with the ecosystem and is rather unintuitive.

This PR aims to fix that by reversing the direction.

Fixes: nodejs#20897
@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label May 24, 2018
@targos
Copy link
Member

targos commented May 24, 2018

I'm surprised no tests have to be updated?

@ryzokuken
Copy link
Contributor Author

I'd be too, I didn't try running and updating tests yet, but will do that today. That said, I hope the format looks good to you?

@targos
Copy link
Member

targos commented May 24, 2018

SGTM

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

-1. This change is not something we should do without having a proper longer discussion about it. The original implementation had a reason and there are quite a few factors to anticipate.

@addaleax
Copy link
Member

@BridgeAR I opened an issue for that very reason. Could you weigh in there? So far me + 4 other people feel this way and nobody has disagreed yet (it’s only been open for 2 days, though).

If you say “the original implementation had a reason and there are quite a few factors to anticipate” it would be great to know that reason and those factors :)

@BridgeAR
Copy link
Member

I am still tying ;-)

@BridgeAR
Copy link
Member

In general I am not -1 on the actual change. More like a +-0. I just think that we should all agree on it first in the issue. Besides the fact that this PR is not even remotely done (there are lots of tests that are going to fail because of such a change).

@BridgeAR BridgeAR added the wip Issues and PRs that are still a work in progress. label May 24, 2018
@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 24, 2018
@ryzokuken
Copy link
Contributor Author

@BridgeAR agreed. Will make the required changes to our testing suite as soon as I can :)

@addaleax
Copy link
Member

Hm. Ping @ryzokuken?

A friend just ran into this issue (assert diffs being the “wrong” way around) and pinged me about it 😕

@BridgeAR
Copy link
Member

@addaleax that change is included in #21628. There are more changes in there though.

@ryzokuken I can remove that part again if you would like to land this PR first. But in that case I would like to also change the color and the tests have to be fixed. I could then rebase accordingly.

@ryzokuken
Copy link
Contributor Author

@BridgeAR agree with the additional changes you have listed. Let's come up with a solid list of TODOs to take care of before landing this and I'd gladly make the changes promptly.

@ryzokuken
Copy link
Contributor Author

Closing in favor of #21628, which already addresses the said issue.

@ryzokuken ryzokuken closed this Jul 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

assert Issues and PRs related to the assert subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. wip Issues and PRs that are still a work in progress.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

assert diffs are in the wrong direction

6 participants