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

feat: split triggers in matcher.rs into categories with explanations #636

Merged
merged 2 commits into from
Feb 10, 2025

Conversation

hippietrail
Copy link
Contributor

I know the logic here is up for replacement but I think this categorization will still help in various ways:

  1. It will help elucidate different classes and types of replacements.

  2. It will help us see which categories will be amenable to various kinds of automation.

  3. It will help curators adding more lints in the meantime.

  4. It also shows that some lints combine multiple kinds of problems while others don't fit well into any neat category. There is some fuzziness.

(I first sorted them alphabetically then grouped them, so the groups themselves are more in a kind of alphabetical order than any semantic order.)

I know the logic here is up for replacement but I think this categorization will still help in various ways:

It will help elucidate different classes and types of replacements.

It will help us see which categories will be amenable to various kinds of automation.

It will help curators adding more lints in the meantime.

It also shows that some lints combine multiple kinds of problems while others don't fit well into any category. There is some fuzziness.

I first sorted them alphabetically then grouped them, so the groups themselves are more in a kind of alphabetical order than any semantic order.
@elijah-potter
Copy link
Collaborator

elijah-potter commented Feb 10, 2025

This PR isn't super helpful. I appreciate the thought, but it feels like busy work.

Your changes also significantly increase the construction cost of the Matcher by allocating more than the previous implementation. It's an acceptable cost under most circumstances, but it underlines why reviewing this kind of change is non-trivial.

I'll merge it, but next time include it in another PR so I can review it as a whole, rather than as a part.

That said, you've been incredibly helpful to the development of Harper. I appreciate it immensely. Keep up the good work!

@elijah-potter elijah-potter merged commit 058306a into Automattic:master Feb 10, 2025
17 checks passed
@hippietrail
Copy link
Contributor Author

This PR isn't super helpful. I appreciate the thought, but it feels like busy work.

It was partly because in testing things kept leading me here so I wanted to understand it to see what kind of replacements are done where and add more of the same. For instance it wasn't clear to me before this that what could be considered diverse jobs were all done in one place.

Your changes also significantly increase the construction cost of the Matcher by allocating more than the previous implementation. It's an acceptable cost under most circumstances, but it underlines why reviewing this kind of change is non-trivial.

Ah I wasn't sure about that. I could also do it in one function call with just comments interspersed to describe the sections.

I'll merge it, but next time include it in another PR so I can review it as a whole, rather than as a part.

I'm not sure what you about in another PR or what it's part of now. Sorry.

That said, you've been incredibly helpful to the development of Harper. I appreciate it immensely. Keep up the good work!

Thank you! So far it keeps inspiring me with more ideas, and something real to properly learn Rust.

@hippietrail hippietrail deleted the matcher-categories branch February 10, 2025 16:07
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Feb 11, 2025
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [Automattic/harper/harper-ls](https://github.com/Automattic/harper) | minor | `v0.19.1` -> `v0.20.0` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>Automattic/harper (Automattic/harper/harper-ls)</summary>

### [`v0.20.0`](https://github.com/Automattic/harper/releases/tag/v0.20.0)

[Compare Source](Automattic/harper@v0.19.1...v0.20.0)

#### What's Changed

-   chore: Maintenance Changes by [@&#8203;mcecode](https://github.com/mcecode) in Automattic/harper#583
-   build: use `just`-native dependency resolution by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#554
-   build(deps): bump tree-sitter-dart from 0.0.3 to 0.0.4 by [@&#8203;dependabot](https://github.com/dependabot) in Automattic/harper#566
-   build(deps): bump serde_json from 1.0.137 to 1.0.138 by [@&#8203;dependabot](https://github.com/dependabot) in Automattic/harper#568
-   build(deps-dev): bump [@&#8203;rollup/plugin-typescript](https://github.com/rollup/plugin-typescript) from 11.1.6 to 12.1.2 in /packages by [@&#8203;dependabot](https://github.com/dependabot) in Automattic/harper#569
-   feat(web): add link to the Discord server by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#585
-   clean up strange character at end-of-line by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#580
-   feat(core): Lint for those who don't like Oxford commas by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#562
-   docs(obsidian): notify people they should use up-to-date Electron by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#586
-   build(deps-dev): bump vitest from 2.1.8 to 2.1.9 in /packages by [@&#8203;dependabot](https://github.com/dependabot) in Automattic/harper#588
-   build(deps-dev): bump [@&#8203;vitest/browser](https://github.com/vitest/browser) from 2.1.8 to 2.1.9 in /packages by [@&#8203;dependabot](https://github.com/dependabot) in Automattic/harper#591
-   fix(core): add `on` to the list of special cases by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#592
-   feat(core): start support for hex numbers by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#553
-   feat: change `just addnoun` to have different affixes as per [#&#8203;595](Automattic/harper#595) by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#601
-   Phrase fixes by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#589
-   dict: add inclusivity by [@&#8203;lukasmwerner](https://github.com/lukasmwerner) in Automattic/harper#590
-   docs(vscode): revised introductional material by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#602
-   docs: missing word in web/author-a-rule by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#614
-   feat: adds a brief helpful comment to each entry describing its function by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#593
-   fix(ls): release config lock to avoid deadlocks by [@&#8203;nyonson](https://github.com/nyonson) in Automattic/harper#612
-   feat(core): create rule for the possessive version of "you" by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#619
-   bench(harper.js): add benchmarks for configuration methods by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#618
-   grammar agreement in obsidion plugin comment by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#622
-   feat: clarify message for uncapitalized "I" by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#625
-   build(deps-dev): bump [@&#8203;types/node](https://github.com/types/node) from 22.13.0 to 22.13.1 in /packages by [@&#8203;dependabot](https://github.com/dependabot) in Automattic/harper#632
-   build(deps-dev): bump esbuild from 0.24.2 to 0.25.0 in /packages by [@&#8203;dependabot](https://github.com/dependabot) in Automattic/harper#633
-   build(deps-dev): bump typescript from 5.6.3 to 5.7.3 in /packages by [@&#8203;dependabot](https://github.com/dependabot) in Automattic/harper#631
-   build(deps-dev): bump [@&#8203;sveltejs/kit](https://github.com/sveltejs/kit) from 2.16.1 to 2.17.1 in /packages by [@&#8203;dependabot](https://github.com/dependabot) in Automattic/harper#629
-   chore: correct some affix annotations by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#617
-   feat: split triggers in `matcher.rs` into categories with explanations by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#636
-   Cut-and-paste error in "report error" template by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#623
-   bug: reverse correcting "supposed to" to "suppose to" by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#621
-   feat: if spellcheck only has one suggestion mention it directly by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#626
-   fix(obsidian): correctly handle Unicode conversions by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#627
-   feat: add `harper-cli forms` and `just getforms` by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#615
-   Closed Compound Matcher Conversions by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#608
-   build(deps): bump clap from 4.5.27 to 4.5.28 by [@&#8203;dependabot](https://github.com/dependabot) in Automattic/harper#635
-   build(deps): bump esbuild from 0.23.1 to 0.25.0 in /packages by [@&#8203;dependabot](https://github.com/dependabot) in Automattic/harper#639
-   fix(core): make narrow linter specifically for `long and behold` by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#641
-   fix(core): currency placement with certain decimal positions by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#604

#### New Contributors

-   [@&#8203;nyonson](https://github.com/nyonson) made their first contribution in Automattic/harper#612

**Full Changelog**: Automattic/harper@v0.19.1...v0.20.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS4xNjQuMSIsInVwZGF0ZWRJblZlciI6IjM5LjE2NC4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
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.

2 participants