Skip to content
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.

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

Improve naming of toggleAnalyzer parameter #55

Merged
merged 4 commits into from
Sep 7, 2023

Conversation

Staijn1
Copy link
Contributor

@Staijn1 Staijn1 commented Sep 4, 2023

Hey!

I found myself wondering if the parameter of toggleAnalyzer enabled, or disabled the analyzer.
The only thing this PR does is change it's name to be more descriptive, and add some comments.

I don't think the README.md needs to be updated.

@Staijn1 Staijn1 changed the title Develop Improve naming of toggleAnalyzer parameter Sep 4, 2023
@hvianna
Copy link
Owner

hvianna commented Sep 5, 2023

Hey @Staijn1 !

Thanks for the PR. I agree the parameter name should be more clear (I suck at choosing variable names 😅), but I think mustEnable can be misleading as well, because when it's set to false it will actually stop the analyzer.

For similar native methods, force seems to be a standard parameter name in the JavaScript docs:

https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/togglePopover
https://developer.mozilla.org/en-US/docs/Web/API/Element/toggleAttribute
https://developer.mozilla.org/en-US/docs/Web/API/DOMTokenList/toggle

And jQuery uses state for this parameter name in a similar method, which also seems like a good choice.

https://api.jquery.com/toggleClass/#toggleClass-className-state

What do you think?

@Staijn1
Copy link
Contributor Author

Staijn1 commented Sep 5, 2023

I will admit, I was not completely sold on mustEnable either..

I asked my, (maybe our) best friend ChatGPT for any other suggestions and he came back with the following:

  • activateAnalyzer
  • desiredState
  • activate
  • enableAnalyzer
  • shouldEnable

However, judging by the MDN documentation I think we should go with force to be consistent with native Javascript functions. Although activate in my opinion is not bad either.

@hvianna
Copy link
Owner

hvianna commented Sep 5, 2023

Sticking with force seems good to me!

@Staijn1
Copy link
Contributor Author

Staijn1 commented Sep 6, 2023

Suggested changes are pushed! :)

@hvianna hvianna merged commit 764dab2 into hvianna:develop Sep 7, 2023
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.

None yet

2 participants