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

example for adding a new rule #494

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

hippietrail
Copy link
Contributor

as per elijah-potter's request

See there for details on how I believe I've followed the instructions but my new rules is ignored and trying it only results in this:

hippietrail@macbookair harper % just lint ~/harper-test/harper-test.md
cargo run --bin harper-cli -- lint /Users/hippietrail/harper-test/harper-test.md
warning: /Users/hippietrail/harper/harper-cli/Cargo.toml: unused manifest key: package.private
warning: /Users/hippietrail/harper/harper-wasm/Cargo.toml: unused manifest key: package.private
   Compiling harper-core v0.16.0 (/Users/hippietrail/harper/harper-core)
   Compiling harper-tree-sitter v0.16.0 (/Users/hippietrail/harper/harper-tree-sitter)
   Compiling harper-typst v0.16.0 (/Users/hippietrail/harper/harper-typst)
   Compiling harper-html v0.16.0 (/Users/hippietrail/harper/harper-html)
   Compiling harper-comments v0.16.0 (/Users/hippietrail/harper/harper-comments)
   Compiling harper-literate-haskell v0.16.0 (/Users/hippietrail/harper/harper-literate-haskell)
   Compiling harper-cli v0.1.0 (/Users/hippietrail/harper/harper-cli)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 4.50s
     Running `target/debug/harper-cli lint /Users/hippietrail/harper-test/harper-test.md`
Advice: 
   ╭─[harper-test.md:1:1]
   │
 1 │ To reiterate, that that is cool is not uncool.
   │               ────┬────  
   │                   ╰────── “that that” sometimes means “that which”, which is clearer.
   │ 
 3 │ To reiterate, foo foo is cool is not uncool.
   │               ───┬───  
   │                  ╰───── Did you mean to repeat this word?
───╯

The added rule should look like the that-which rule but silly.

@elijah-potter
Copy link
Collaborator

@hippietrail, I've compiled your branch and it looks like it's working just fine.

Until now, if more than one rule finds an error harper-cli will only show the first one. If you open it up in Visual Studio Code or (in my case) Neovim, it shows both lints just fine.

That's actually a bug, and I've pushed a commit that should fix it 🤞. Now if you test using harper-cli, it'll show the FooBar.

@hippietrail
Copy link
Contributor Author

hippietrail commented Jan 24, 2025

@hippietrail, I've compiled your branch and it looks like it's working just fine.

Until now, if more than one rule finds an error harper-cli will only show the first one. If you open it up in Visual Studio Code or (in my case) Neovim, it shows both lints just fine.

That's actually a bug, and I've pushed a commit that should fix it 🤞. Now if you test using harper-cli, it'll show the FooBar.

   ╭─[harper-test.md:1:1]
   │
 1 │ To reiterate, that that is cool is not uncool.
   │               ────┬────  
   │                   ╰────── “that that” sometimes means “that which”, which is clearer.
   │ 
 3 │ To reiterate, foo foo is cool is not uncool.
   │               ───┬───  
   │                  ╰───── “foo foo” sometimes means “foo bar”, bar is clearer.
───╯

OK nice. What about something like a --verbose switch that will show all matching rules and not just the first? As the number of rules grow there's going to a geometric increase in hidden ambiguities resulting in one rule chosen over others leading to such surprises, as anyone who has experimented with recursive descent parsing or parser combinators can attest.

Wait a sec... didn't you just say it should show both now?

I'll see if I can manage to test via LSP... Nope, sorry. Is that also in the new review docs? I can only see how to test via harper-cli and something about Docker that has a link that says it's for a demo but just takes me to part of a github repo??

@elijah-potter
Copy link
Collaborator

OK nice. What about something like a --verbose switch that will show all matching rules and not just the first?

I've set that up. You can either use just lint-verbose <file> or cargo run --bin harper-cli -- <file> --verbose

Are you happy enough with your FooBar rule that you want to elevate it to a real PR?

@hippietrail
Copy link
Contributor Author

OK nice. What about something like a --verbose switch that will show all matching rules and not just the first?

I've set that up. You can either use just lint-verbose <file> or cargo run --bin harper-cli -- <file> --verbose

Are you happy enough with your FooBar rule that you want to elevate it to a real PR?

Not yet because though it's working I still don't grok everything. In particular I don't understand the function of all the "that"/"foo" lines I mentioned in #501:
Image

I'm looking in the great news docs but I'm not sure if this is missing or I'm just not seeing it. I can see that the pattern.add()s are adding two casings of a word to "A pattern collection to look for patterns that start with a specific word." with "A pattern that checks that a sequence of other patterns match." as the other argument ... but I don't know what that means.

By the way, I have more questions in my other PR for a real lint: #502

I'm also stuck on another real lint I've started. I could also do a work-in-progress PR for that one if you like in the hope that me asking all the dumb questions now will make it so future contributors won't have to ask them again (-:

@hippietrail
Copy link
Contributor Author

OK nice. What about something like a --verbose switch that will show all matching rules and not just the first?

I've set that up. You can either use just lint-verbose <file> or cargo run --bin harper-cli -- <file> --verbose

That's great! But now I can see that with or without --verbose it only shows the Lint.messages but doesn't show the suggestions. Should it do both or should that be a separate option? I haven't found another way to test suggestions from the commandline so please let me know if there's a way that I've missed.

@elijah-potter
Copy link
Collaborator

I haven't found another way to test suggestions from the commandline so please let me know if there's a way that I've missed.

Honestly, the best way to test suggestions is in an actual editor (like Visual Studio Code). Have you tried running your own build in there?

@hippietrail
Copy link
Contributor Author

hippietrail commented Jan 28, 2025

I haven't found another way to test suggestions from the commandline so please let me know if there's a way that I've missed.

Honestly, the best way to test suggestions is in an actual editor (like Visual Studio Code). Have you tried running your own build in there?

I haven't figured out how to do that.
I spoke to soon. I think I've got it set up now...

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