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

Refactor: update JSDoc for subdomains getter and simplify implementation #6135

Conversation

Ayoub-Mabrouk
Copy link
Contributor

  • Changed JSDoc return type to specify an array of strings.
  • Removed the unnecessary function name from the subdomains getter for better readability.
  • Used destructuring to simplify property access and enhance clarity in the code.

- Changed JSDoc return type to specify an array of strings.
- Removed the unnecessary function name from the subdomains getter for better readability.
- Used destructuring to simplify property access and enhance clarity in the code.
Copy link
Member

@wesleytodd wesleytodd left a comment

Choose a reason for hiding this comment

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

While I am usually glad to improve the jsdoc, the rest of this is just style. and cannot be accepted. If you would like to go and make PRs that just update the jsdoc, it is much more likely those will be merged.

@wesleytodd wesleytodd closed this Jan 15, 2025
@Ayoub-Mabrouk
Copy link
Contributor Author

While I am usually glad to improve the jsdoc, the rest of this is just style. and cannot be accepted. If you would like to go and make PRs that just update the jsdoc, it is much more likely those will be merged.

Thank you @wesleytodd for your feedback. I believe the changes go beyond being purely stylistic. By using destructuring, the code reduces repetition and improves clarity, while the switch to const ensures immutability and aligns with modern best practices. Additionally, the simplified isIP condition makes the logic easier to follow, and removing the unnecessary named function avoids potential confusion. These adjustments aim to enhance readability, maintainability, and consistency with modern JavaScript standards, which can be beneficial in the long run.

@bjohansebas
Copy link
Member

Probably all those style changes can be delegated to neostandard or a similar linter

@wesleytodd
Copy link
Member

If they were done as part of adopting a uniform linting config I would happily approve that PR. As it stands, what you describe as improvements are at worst non-goals and at best subjective improvements. I have replied to a few of these things in the other PRs, but will repeat a few reasons here:

using destructuring, the code reduces repetition and improves clarity

Repeating this. twice is not a problem. And as this is subjective, I would actually consider this.hostname repeated a few times to be more clear since it doesn't add an unnecessary rename to understand when reading the code. Subjective means "style".

the switch to const ensures immutability and aligns with modern best practices

The code is well contained and not changed often, this is a subjective "improvement" with no other impact. Hence "style".

removing the unnecessary named function avoids potential confusion

Named functions are critical for both error debugging and performance investigations. We should be adding names where they are missing, not removing them.

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.

3 participants