Skip to content

feat(no-debug): support utilNames option #357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 10 commits into from
May 13, 2021
Merged

feat(no-debug): support utilNames option #357

merged 10 commits into from
May 13, 2021

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Apr 26, 2021

Fixes half of #294

Should be followed up with creating a new rule called no-debugging-utils that this one is deprecated for.

I've gone with the very average "utilNames" name for the option - very happy to change it.
I've also left isDebugUtil in detect-testing-library-utils even though it looks like its now unused, as I don't know if you're publishing or exposing it for third-parties to use? (so removing it might be breaking?)

I've done this as a first pass implementation, so happy to make changes to docs, names, etc as desired :)

(this is also why I've not refactored the noDebug message to be noDebugUtils etc, in case this PR is going in the wrong direction)

@Belco90
Copy link
Member

Belco90 commented Apr 26, 2021

Oh wow, thank you very much @G-Rath! I'll take a deeper look during the week. The direction of the PR seems ok in general, but there are a couple of things I'd like to discuss (like utilNames option vs dedicated option per debug util name, or reusing isDebugUtil with a 2nd param validNames like isAsyncUtil accepting Array<'debug' | 'logTestingPlaygroundURL'>

@Belco90 Belco90 self-requested a review April 26, 2021 10:16
Belco90
Belco90 requested changes Apr 29, 2021
View reviewed changes
Copy link
Member

@Belco90 Belco90 left a comment

Choose a reason for hiding this comment

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

Review finished! I requested few changes so:

  • we reuse isDebugUtil to be able to detect logTestingPlaygroundURL
  • we opt-in reporting logTestingPlaygroundURL by default

Thanks!

@G-Rath G-Rath requested a review from Belco90 April 30, 2021 04:06
Belco90
Belco90 requested changes Apr 30, 2021
View reviewed changes
Copy link
Member

@Belco90 Belco90 left a comment

Choose a reason for hiding this comment

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

Looking good! Just a couple of small tweaks and should be ready to go. Thanks!

MichaelDeBoey
MichaelDeBoey previously requested changes Apr 30, 2021
View reviewed changes
Copy link
Member

@MichaelDeBoey MichaelDeBoey left a comment

Choose a reason for hiding this comment

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

This one's looking good, thanks @G-Rath! 👊

Added a couple of tweaks that you need to do, but other than that this one looks good for me.

I'm still not sure about the array vs. object (I can see the benefit of both of them) and the fact if we should make it breaking or not in this case though. 🤔

@G-Rath
Copy link
Contributor Author

G-Rath commented Apr 30, 2021

@Belco90 I'm keen to add a few more debug utils to the list of what's supported (but keeping the current defaults, unless you'd like them added there too), if that's alright with you?

Specifically, the ones @MichaelDeBoey mentioned here.

@MichaelDeBoey
Copy link
Member

@G-Rath I would be OK with adding the other ones I mentioned in #294 (comment).

@MichaelDeBoey MichaelDeBoey added the enhancement New feature or request label May 1, 2021
@Belco90
Copy link
Member

Belco90 commented May 2, 2021

@Belco90 I'm keen to add a few more debug utils to the list of what's supported (but keeping the current defaults, unless you'd like them added there too), if that's alright with you?

Specifically, the ones @MichaelDeBoey mentioned here.

By all means! Also, regarding the way of declaring the options as array vs boolean properties, I think you are right and the latter is better for the final user without implying more work to maintain, so I'd be happy to go ahead with that too.

Sorry for being late confirming this, and thanks for your patience :)

@G-Rath G-Rath requested review from Belco90 and MichaelDeBoey May 6, 2021 23:24
Belco90
Belco90 requested changes May 7, 2021
View reviewed changes
Copy link
Member

@Belco90 Belco90 left a comment

Choose a reason for hiding this comment

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

Looking amazing! The last necessary change is to update the rule doc accordingly with the final implementation and should be ready to go. Thanks!

@G-Rath G-Rath requested a review from Belco90 May 13, 2021 21:06
@G-Rath
Copy link
Contributor Author

G-Rath commented May 13, 2021

@Belco90 have updated the docs.

on an aside, I wrote a script for automatically keeping rule docs consistent across rule tables in the README, the header in each rules doc, and the meta.docs.description property for each rule - you're welcome to take it if you'd find that useful to have here :) (I'm also happy to help set it up too if you like).

Belco90
Belco90 requested changes May 13, 2021
View reviewed changes
Copy link
Member

@Belco90 Belco90 left a comment

Choose a reason for hiding this comment

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

on an aside, I wrote a script for automatically keeping rule docs consistent across rule tables in the README, the header in each rules doc, and the meta.docs.description property for each rule - you're welcome to take it if you'd find that useful to have here :) (I'm also happy to help set it up too if you like).

That's awesome! We are actually doing some improvements around this, so we will get inspiration from your script for sure. Thanks!

@G-Rath G-Rath requested a review from Belco90 May 13, 2021 23:32
Belco90
Belco90 approved these changes May 13, 2021
View reviewed changes
Copy link
Member

@Belco90 Belco90 left a comment

Choose a reason for hiding this comment

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

Fantastic! Thank you very much for your contribution, and sorry for the initial back and forth.

@G-Rath
Copy link
Contributor Author

G-Rath commented May 13, 2021

No problem - happy to help a fellow eslint plugin 😄

@Belco90 Belco90 dismissed MichaelDeBoey’s stale review May 13, 2021 23:37

Changes addressed already.

@Belco90 Belco90 merged commit b2579bb into testing-library:main May 13, 2021
@Belco90
Copy link
Member

Belco90 commented May 13, 2021

@all-contributors please add @G-Rath for code, docs and test

@allcontributors allcontributors bot mentioned this pull request May 13, 2021
Merged
@allcontributors
Copy link
Contributor

@Belco90

I've put up a pull request to add @G-Rath! 🎉

@github-actions
Copy link

🎉 This PR is included in version 4.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@G-Rath G-Rath deleted the no-debug-support-utils-option branch May 13, 2021 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Belco90 Belco90 Belco90 approved these changes

@MichaelDeBoey MichaelDeBoey Awaiting requested review from MichaelDeBoey

Assignees

@G-Rath G-Rath

Labels
enhancement New feature or request released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@G-Rath @Belco90 @MichaelDeBoey