Skip to content

docs: added page to explain tooling for reviewers#471

Merged
elijah-potter merged 3 commits intomasterfrom
branch-install-docs
Jan 23, 2025
Merged

docs: added page to explain tooling for reviewers#471
elijah-potter merged 3 commits intomasterfrom
branch-install-docs

Conversation

@elijah-potter
Copy link
Copy Markdown
Collaborator

In #455, it became apparent that there is currently a lack of documentation for how to build and test patches to Harper. In response, I've written a short page for contributors that hopefully clear some things up.

I'd love to know what's missing from this page. Is there anything that needs further clarification or revision?

@elijah-potter elijah-potter added the documentation Improvements or additions to documentation label Jan 22, 2025
@elijah-potter elijah-potter self-assigned this Jan 22, 2025
Comment on lines +25 to +29
For example, for [PR #445](https://github.com/Automattic/harper/pull/455), we can install the patched version of the `harper-cli` debug tool with the following command:

```bash
cargo install --git https://github.com/automattic/harper --branch somewhat-something harper-cli
```
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is something missing here, somewhere-somewhat is the name of the branch in 455.

If you don't explain it, somewhat-somewhere sounds like a meta syntaxic words like "foo-bar"

I'm reporting it, because you lost me.
I had to read carefully 5 times before understanding

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think I'm even more lost here than you are -:


## Using Cargo

If you only have [Cargo](https://doc.rust-lang.org/cargo/) installed, you can compile either `harper-ls` (the language server) or `harper-cli` (our command-line debug tool) directly from the branch in GitHub:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this a follow-on from somewhere previous if I had not just Cargo installed but something more powerful as well/instead?

(am I supposed to click "Add single comment" or "Start a review" here? Going with the former after Copilot failed to help.)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

(am I supposed to click "Add single comment" or "Start a review" here? Going with the former after Copilot failed to help.)

Either works.

Is this a follow-on from somewhere previous if I had not just Cargo installed but something more powerful as well/instead?

I'm a little confused by your meaning.

We use just to orchestrate the various build tools we use for different parts of the project (i.e. cargo for Rust, yarn for JavaScript).
Since you don't necessarily need to build any of the JavaScript stuff to test harper-core, we can just use cargo directly.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sorry I'm not talking about the tool "just". I'm confused by the wording about only having Cargo and used "just" as a synonym of "only" in a failed attempt to clarify what I was confused by.

Oh maybe it's saying something like "you don't need all the Javascript stuff to test Harper, you only need Caro"?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Oh maybe it's saying something like "you don't need all the Javascript stuff to test Harper, you only need Caro"?

Precisely

@elijah-potter
Copy link
Copy Markdown
Collaborator Author

I'm working on improving my ability to write logical documentation. I've given it another go-round. Does it make more sense now?

@hippietrail
Copy link
Copy Markdown
Collaborator

I'm working on improving my ability to write logical documentation. I've given it another go-round. Does it make more sense now?

Yes that's great! Much easier to follow now. Don't worry, everybody knows how hard writing documentation is.

@elijah-potter
Copy link
Copy Markdown
Collaborator Author

Thanks @hippietrail. In that case, I'm going to merge it.

@elijah-potter elijah-potter merged commit 993aedd into master Jan 23, 2025
@elijah-potter elijah-potter deleted the branch-install-docs branch January 23, 2025 03:56
@hippietrail
Copy link
Copy Markdown
Collaborator

hippietrail commented Jan 23, 2025

Thanks @hippietrail. In that case, I'm going to merge it.

Thank you! Hopefully I'll get to try it out this afternoon so I'll let you know how I go.


Update after trying to add a my_rule.rs

After my initial confusion, I think I managed to edit all the related files by searching for that_which and adding a modified duplicate line after each for my_rule but now I get to the cargo line for testing and it seems to want a URL to a branch. Does this mean that I have to commit my code before I can test it? Or am I missing something?


I think I'm getting confused now because the docs are specifically for testing a new published feature in a branch but at the same time I've also been looking at how to implement my own new lint and test that.

I'm not sure both are laid out in full in the docs. I believe that to test my own change I can do cargo run --bin harper-cli but then I need to have a file ready. I tried -- to work with stdin. Maybe that's possible somehow or I could file a feature request.

@elijah-potter
Copy link
Copy Markdown
Collaborator Author

I'm not sure both are laid out in full in the docs. I believe that to test my own change I can do cargo run --bin harper-cli but then I need to have a file ready. I tried -- to work with stdin. Maybe that's possible somehow or I could file a feature request.

I've added some documentation in #482 on this. Mind taking a look?

@hippietrail
Copy link
Copy Markdown
Collaborator

I'm not sure both are laid out in full in the docs. I believe that to test my own change I can do cargo run --bin harper-cli but then I need to have a file ready. I tried -- to work with stdin. Maybe that's possible somehow or I could file a feature request.

I've added some documentation in #482 on this. Mind taking a look?

Good work thanks. I made a couple of suggestions. And I finally installed just. But I still can't get my new rule to work though.

tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Jan 28, 2025
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [Automattic/harper/harper-ls](https://github.com/Automattic/harper) | minor | `v0.16.0` -> `v0.18.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.18.0`](https://github.com/Automattic/harper/releases/tag/v0.18.0)

[Compare Source](Automattic/harper@v0.17.0...v0.18.0)

#### What's Changed

-   fix two grammatical errors by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#481
-   Reword description of "that which" to avoid ironic redundancy by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#479
-   feat([#&#8203;422](Automattic/harper#422)): add `same than` -> `same as` matcher trigger by [@&#8203;grantlemons](https://github.com/grantlemons) in Automattic/harper#453
-   missing definite article, comma placement by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#480
-   Improvements to `PluralConjugate` by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#474
-   correct Valentine's Day by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#491
-   typo in zed integration doc by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#490
-   feat(core): created linter for [#&#8203;426](Automattic/harper#426) by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#452
-   I missed a lets/let's mistake in a comment by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#492
-   docs: rewrote instructions on how to author a rule by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#482
-   missing indefinite article in comments by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#488
-   Apply rule from issue [#&#8203;465](Automattic/harper#465) to core document.rs by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#510
-   feat(core): allow trailing commas in the `lint_group` macro by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#483
-   fix agreement error in docs by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#417

**Full Changelog**: Automattic/harper@v0.17.0...v0.18.0

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

[Compare Source](Automattic/harper@v0.16.0...v0.17.0)

#### What's Changed

-   feat([#&#8203;393](Automattic/harper#393)): add clap version & about attributes by [@&#8203;grantlemons](https://github.com/grantlemons) in Automattic/harper#394
-   docs: added Homebrew as an installation method by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#395
-   typos/spelling/grammar fixes in comments, docs, and strings by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#398
-   fix(core): harden title case module by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#396
-   fix(core): indexing problems by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#409
-   fix typo: remove extraneous 'to' by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#418
-   chore: update description of Oxford rule by [@&#8203;ccoVeille](https://github.com/ccoVeille) in Automattic/harper#419
-   build(deps): bump itertools from 0.13.0 to 0.14.0 by [@&#8203;dependabot](https://github.com/dependabot) in Automattic/harper#431
-   build(deps): bump serde_json from 1.0.135 to 1.0.137 by [@&#8203;dependabot](https://github.com/dependabot) in Automattic/harper#429
-   Typst Test Fixes by [@&#8203;grantlemons](https://github.com/grantlemons) in Automattic/harper#391
-   build(deps): bump clap from 4.5.23 to 4.5.27 by [@&#8203;dependabot](https://github.com/dependabot) in Automattic/harper#444
-   feat: Update VSCode Config by [@&#8203;mcecode](https://github.com/mcecode) in Automattic/harper#400
-   fix(core): infinite lint loops by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#390
-   Clean Up `harper-ls` logs by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#447
-   build(deps): bump undici from 6.19.8 to 6.21.1 in /packages by [@&#8203;dependabot](https://github.com/dependabot) in Automattic/harper#450
-   typo by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#462
-   fix(core): the Oxford comma applies to `nor` as well by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#445
-   feat(harper.js): added ability to explicitly set config to the default by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#449
-   build(deps-dev): bump vite from 6.0.5 to 6.0.9 in /packages by [@&#8203;dependabot](https://github.com/dependabot) in Automattic/harper#448
-   feat(core): created a  new rule that resolves [#&#8203;414](Automattic/harper#414) by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#455
-   build(deps-dev): bump vite from 5.4.11 to 5.4.12 in /packages by [@&#8203;dependabot](https://github.com/dependabot) in Automattic/harper#475
-   docs: added page to explain tooling for reviewers by [@&#8203;elijah-potter](https://github.com/elijah-potter) in Automattic/harper#471
-   fix instance of [#&#8203;384](Automattic/harper#384) in docs by [@&#8203;hippietrail](https://github.com/hippietrail) in Automattic/harper#477

#### New Contributors

-   [@&#8203;hippietrail](https://github.com/hippietrail) made their first contribution in Automattic/harper#398
-   [@&#8203;ccoVeille](https://github.com/ccoVeille) made their first contribution in Automattic/harper#419

**Full Changelog**: Automattic/harper@v0.16.0...v0.17.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:eyJjcmVhdGVkSW5WZXIiOiIzOS4xMjYuMSIsInVwZGF0ZWRJblZlciI6IjM5LjEzNC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants