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

Some improvements and thoughts #325

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

EAzari
Copy link
Contributor

@EAzari EAzari commented Mar 29, 2023

No description provided.

@krasimir
Copy link
Owner

Hey, thank you for the PR but can you provide some more description what exactly you are changing and why.

@EAzari
Copy link
Contributor Author

EAzari commented Mar 30, 2023

I just tried to rewrite it to learn about routers, and tried make it more type safe, and morereadable, modern and usable.
I think your codes are great enough, I just tried improve them a little bit, and share some thoughts in comments
That's it

@tekhedd
Copy link

tekhedd commented Apr 17, 2023

(Sorry for the driveby code review, I found myself browsing through the PRs while integrating Navigo with a project.)

tl;dr It's good code, but, and everything after this comma is going to be negative I'm sorry to say, it seems entirely unnecessary as it includes no functional changes and does not (IMO) improve readability, nor does it appear to be necessary to bring the code closer to the project style conventions. Aside from adding type descriptions, this is a batch of "changes for the sake of changes to match my preferred style". I would suggest resubmitting as separate "type safety update" and "language moderniztaion update" PRs to be evaluated separately, perhaps, as the type safety changes will likely be welcomes.

Much Longer; Did Read Section:

Type declarations are always good! But why the code changes? Replacing ?: with ?? does nothing other than make sure that future maintainers are intimate with the same cool language features you are. Similarly, does every accessor need to be an inline arrow function? Other than that it's cool?

The commit comment is insufficient. To be fair, github has a culture of zero-info commit comments, so that itself is not unusual. That said, commit comments should fully summarize all changes in a commit, so you should not actually need to read the code to understand what it does. Reading the code is just verification that the comment is accurate.

"improvements and thoughts": All of the changes are (unnecessary?) stylistic changes. Full stop. A few comments to save future maintainers from have to spend the same time you did to understand the existing code could, perhaps, have been of value.

It's not my time to spend, and I realize saying this is annoying and condescending, but I'm going to say it anyway: the time spent on these "modernization" PRs could have closed an actual issue. The best way to learn how to write maintainable code is to go through the hell of maintaining someone's code. It's much harder to conform to someone else's style--updating it to what you think is best is, well, easy.

Anyway, sorry if this seems very negative...there's nothing wrong with the code. But unnecessary changes just introduce the risk of bugs, and I think it's rude to put social pressure on a project maintainer by submitting PRs that, rather than helping to close issues, increase the work and add nothing to a project.

As rude as a drive-by code review? Probably not. :) Sorry about that. Great component BTW! l8r!

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

3 participants