Skip to content

Conversation

vemv
Copy link
Member

@vemv vemv commented Dec 10, 2021

  • Unwrap lists for :actual in = arity 2
  • Use traditional clojure.test notation for :actual in = arity 3+

This is clearly reflected in the included tests.

Fixes #735

@vemv vemv requested a review from bbatsov December 10, 2021 05:51
@bbatsov
Copy link
Member

bbatsov commented Dec 10, 2021

Why did you change the PR template?

@vemv
Copy link
Member Author

vemv commented Dec 10, 2021

It seemed cluttered, when I receive an email with someone else's PR, reading the instructions yet again seems tiring.

(Also reflection is now covered by Eastwood)

@bbatsov
Copy link
Member

bbatsov commented Dec 10, 2021

It seemed cluttered, when I receive an email with someone else's PR, reading the instructions yet again seems tiring.

The purpose for this is not to have to explain the instructions again and again to new contributors, that's why I want this as detailed as possible. Please, remove those changes from the commit. I'd also separate the lint changes to a different commit as well.

@bbatsov
Copy link
Member

bbatsov commented Dec 10, 2021

The changes to the test middleware look good.

@vemv
Copy link
Member Author

vemv commented Dec 10, 2021

The purpose for this is not to have to explain the instructions again and again to new contributors

I think contributors can observe the comments - at least that has been my experience maintaining issue templates.

Perhaps we can add a particularly unmistakable prelude?

<!-- New contributors, please read the instructions carefully! -->

@bbatsov
Copy link
Member

bbatsov commented Dec 10, 2021

My observation is that almost no one reads stuff, that's why I like the explicit template. And I think that's the observation of many people, otherwise GitHub would not have added this feature. ;-)

@vemv
Copy link
Member Author

vemv commented Dec 10, 2021

Oh IDK, the template is pretty flexible as to what it can contain.

One also can observe that the template often is entirely skipped, even when it had room to be followed (especially for test coverage).

My line of thinking is that by making it more concise, hopefully people will pay a little more attention to it, else it might look like boilerplate after the nth PR.

Ultimatately I can concede oc, but maybe it's good food for thought?

@bbatsov
Copy link
Member

bbatsov commented Dec 10, 2021

The template is mostly for new/infrequent contributors. Seasoned contributors don't really need it, as they obviously know what's expected.

Ultimatately I can concede oc, but maybe it's good food for thought?

Okay, we can revisit the subject down the road. At any rate - I don't like discussing unrelated things in the same PR. Single focus, quick turnaround.

vemv added 2 commits December 10, 2021 11:46
* Unwrap lists for :actual for `=` arity 2
* Use traditional clojure.test notation for `=` arity 3+

Fixes #735
@vemv vemv merged commit 1cfd596 into master Dec 10, 2021
@vemv vemv deleted the 735 branch December 10, 2021 10:47
@marcomorain
Copy link

thanks @vemv

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.

When a test fails, there is a set of parens around the actual result

3 participants