-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Cleanup README #1672
Cleanup README #1672
Conversation
Use consistent em-dashes, and spacing
In lots of ways, I like this. I like improvements to the consistency of the list, especially ones that basically take stuff that was being kinda done and move it to "and now it all does it". My concern however is that maintenance on this will be a pain in the proverbial. Pretty much everything else that's being maintained quality wise right now on the repo is automated. This is very deliberate as it means I have a hope in heck of keeping up, just by being able to go "if the CI is happy, merging won't be that bad". This PR if merged would make a bunch of improvements, and then probably the improvements would go away over time and I just wouldn't notice. If you can figure out a way to automate this (ideally by improvements to the existing Rust tool, but I'll take whatever), I'd be interested in merging this. |
Once the consistency is there automating the lint will be fairly simple (each line matches a regex). The rationale behind this change was that there is a tool that uses this data for visualization and a bunch of lines don't match the regex in the tool. |
I was a bit unclear before: I won't merge this without the lint being added into the CI. This repo chews up a fair amount of time even with the mostly automated reviews, and things that add to the ongoing manual overhead i.e. checking for this sort of thing in new PRs (even if it's not obligatory, merging this would make me wonder about it) are therefore bad. If however, a lint gets added, that's excellent, as then it's just "read the build logs, see the obvious message, at most point that out to people adding things", which is very much inline with how all the other automation works right now. |
Got it - I'll add this at some point soon |
How's this going? |
- Add descriptions for all missing items - add tldr section to contributing doc to make it easy to link to in linter
Done - I think Merged from the main branch into this branch - I'd probably suggest squash merging this rather than hitting the merge button for a neater history. |
…ace surrounding the separator, cleanup readme
if results.get(&url).is_some() { | ||
if results.contains_key(&url) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is picked up by the clippy deny rule added recently.
Awesome. Thanks for this! |
Use consistent em-dashes, and spacing