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

chore(lib): continuation of v3 refactor (see #117) #120

Merged
merged 13 commits into from
Sep 28, 2024

Conversation

Rolv-Apneseth
Copy link

A continuation of and some fixes for the work started in #117

@Rolv-Apneseth
Copy link
Author

I've pointed this to try and merge into the v3 branch so that #117 will still be a valid reference in the changelog - if you wish me to make a PR directly to main just let me know @jeertmans

@Rolv-Apneseth
Copy link
Author

Also, I have not done any work related to the multiple requests as I figured that should be a separate PR

Copy link
Owner

@jeertmans jeertmans left a comment

Choose a reason for hiding this comment

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

Impressive work @Rolv-Apneseth! I left a few comments here and there, but I think this is generally good. I see a lot of changes in the format: did you run cargo +nightly fmt? Because stable toolchain's format is quite different.

Also, it looks like a few tests are failing (MSRV, doc, ...), so it might be good to have them fixed before I can merge :-)

I think this is fine having a PR to v3, and then I will merge v3 is work is over. I agree that the split logic may be part of another PR :-)

README.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@jeertmans jeertmans added the enhancement New feature or request label Sep 21, 2024
@jeertmans jeertmans changed the title Continuation of v3 refactor chore(lib): continuation of v3 refactor (see #117) Sep 21, 2024
@Rolv-Apneseth
Copy link
Author

did you run cargo +nightly fmt

Ah, no, sorry I will do that. I will also have a look at the other issues later today

@Rolv-Apneseth
Copy link
Author

That should hopefully be better, though I expect some will still fail. Also, I saw that on #117, the languagetool action running on *.rs files was giving a lot of false positives, should I have it only run on *.md files?

@Rolv-Apneseth
Copy link
Author

Anything else you'd like me to do? I'll need help solving those remaining CI issues I think

@jeertmans
Copy link
Owner

I'll give it a deeper look later next week, and let you know @Rolv-Apneseth. Thanks for your work!

@Rolv-Apneseth
Copy link
Author

Great, look forward to hearing from you. I'll just leave notes about 2 of the failing CI:

  1. Benchmark

    Fails to compile the code before these changes, which makes sense - probably have to ignore this one for this PR?

  2. Changelog

    The error leads to the readme of the create-or-update-comment action, which says:

    In public repositories this action does not work in pull_request workflows when triggered by forks. Any attempt will be met with the error, Resource not accessible by integration. This is due to token restrictions put in place by GitHub Actions.

    Guess that can't be done for this PR since it's from a fork.

@jeertmans
Copy link
Owner

Great, look forward to hearing from you. I'll just leave notes about 2 of the failing CI:

  1. Benchmark
    Fails to compile the code before these changes, which makes sense - probably have to ignore this one for this PR?

This is probably just this, I'll double-check that during my review.

  1. Changelog
    The error leads to the readme of the create-or-update-comment action, which says:

    In public repositories this action does not work in pull_request workflows when triggered by forks. Any attempt will be met with the error, Resource not accessible by integration. This is due to token restrictions put in place by GitHub Actions.

    Guess that can't be done for this PR since it's from a fork.

Yes, but I just should update the comment workflow (both for LanguageTool and for benchmarks) to allow failure, and use workflow summary instead, see how I do on another repository.

Copy link
Owner

@jeertmans jeertmans left a comment

Choose a reason for hiding this comment

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

Hi @Rolv-Apneseth, thanks again for your work, and sorry for the delay in the review!

The PR looks great, however I have one last comment before I can merge, see my comment.

For the rest, as you mentioned, we will fix that in another PR :-)

src/api/mod.rs Outdated Show resolved Hide resolved
src/api/mod.rs Outdated Show resolved Hide resolved
src/api/mod.rs Outdated Show resolved Hide resolved
src/api/mod.rs Outdated Show resolved Hide resolved
jeertmans and others added 3 commits September 28, 2024 19:27
Co-authored-by: Jérome Eertmans <jeertmans@icloud.com>
Co-authored-by: Jérome Eertmans <jeertmans@icloud.com>
@jeertmans
Copy link
Owner

Oops, I think the last await needs a manual casting map_err(Into::into) or something like that... I will fix that tomorrow :-)

@Rolv-Apneseth
Copy link
Author

Oops, I think the last await needs a manual casting map_err(Into::into) or something like that... I will fix that tomorrow :-)

All good just did it there, talk to you tomorrow

@jeertmans
Copy link
Owner

Looks like this did the trick! Let's merge this, and discuss tomorrow about what are the next steps, if you are still interested to help of course :-)

Thanks for your help and patience on this one!

@jeertmans jeertmans merged commit 561ab8b into jeertmans:v3 Sep 28, 2024
16 of 20 checks passed
@Rolv-Apneseth
Copy link
Author

Great :) and yes I'm still interested

@Rolv-Apneseth Rolv-Apneseth deleted the v3-continued branch September 28, 2024 21:23
jeertmans added a commit that referenced this pull request Sep 29, 2024
* fix!: adjustments after refactor

* Update README.md

Co-authored-by: Jérome Eertmans <jeertmans@icloud.com>

* docs(changelog): remove mention of support for markdown and typst files for now

* refactor: use `check::Request`, `check::Response` and `check::ResponseWithContext`

* chore: formatting

* fix: minimum Rust version needs to be higher for `clap`

* fix: doc test

* refactor: use crate's result type for the `check` method on `Client`

* refactor: use crate's result type for the `languages` method on `Client`

* Update src/api/mod.rs

Co-authored-by: Jérome Eertmans <jeertmans@icloud.com>

* Update src/api/mod.rs

Co-authored-by: Jérome Eertmans <jeertmans@icloud.com>

* fix: convert `reqwest` error

---------

Co-authored-by: Jérome Eertmans <jeertmans@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants