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

Revised add spaces #516

Merged
merged 20 commits into from Oct 12, 2023
Merged

Conversation

kcphila
Copy link
Contributor

@kcphila kcphila commented Oct 3, 2023

Rewrote the PR based on discussion to provide a global wrapper around all docopt calls. Also added some tests to verify that calls to docopt work both when args are preprocessed (exclusively in style-file) and not (created tests for parsable-R for this example).

Copy link
Owner

@lorenzwalthert lorenzwalthert left a comment

Choose a reason for hiding this comment

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

Thanks for the effort to make a new PR @kcphila, see the comments I made. To fix R cmd check issues, please roxygenise and run the pre-commit hooks that should fix some formatting problems. Pre-commit.ci failed because a binary was not yet available for {curl} from the PPM as the roxygenise hook for some reason depends on {curl} recursively.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
DESCRIPTION Outdated Show resolved Hide resolved
R/testing.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
tests/testthat/test-hooks.R Outdated Show resolved Hide resolved
@lorenzwalthert
Copy link
Owner

lorenzwalthert commented Oct 7, 2023

@kcphila please did you forget to install the pre-commit hooks? precommit::use_precommit(). Because roxygen did not run.you can also run roxygen manually. Also, pre-commit hooks succeeded, so maybe caching fools us for new Rd files… needs separate investigation.

Copy link
Owner

@lorenzwalthert lorenzwalthert left a comment

Choose a reason for hiding this comment

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

Can you fix these style issues and re-document (or let pre-commit.CI do it)?

R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
@kcphila
Copy link
Contributor Author

kcphila commented Oct 11, 2023

Can you fix these style issues and re-document (or let pre-commit.CI do it)?

Hi @lorenzwalthert, looks like you committed the two minor revisions from this morning. Are there any outstanding issues remaining?

@lorenzwalthert
Copy link
Owner

Only that I realised all hook script helper have

#' This function is only exported for use in hook scripts, but it's not intended
#' to be called by the end-user directly.

in their docs. Can you add that? And that the roxygen hook had a regression described in #517, but that’s unrelated to your PR.

@lorenzwalthert
Copy link
Owner

Then we can merge. Sorry I can’t help, away without labtop

@lorenzwalthert
Copy link
Owner

Great. @kcphila as a final test before we merge this, you can already use this in your repo. In your pre-commit config, set the rev tag to the commit 44ed0e2 instead of the latest tag or branch you have there. Then, try to commit something. Pre-commit will first install this very revision and then run the checks and then commit. So I suggest you test this with a file that previously did not work because it has spaces in the file name. If everything works, please confirm that here and I will merge the PR. Ok?

@kcphila
Copy link
Contributor Author

kcphila commented Oct 12, 2023

If everything works, please confirm that here and I will merge the PR. Ok?

Hi @lorenzwalthert, success! I verified it crashing with the 0.3.2 and then updated the yaml config and it styled successfully. Just for sanity I also did the same for a already correctly styled file with artificial spaces in it.

@lorenzwalthert lorenzwalthert merged commit e989416 into lorenzwalthert:main Oct 12, 2023
17 checks passed
@lorenzwalthert
Copy link
Owner

Thanks @kcphila, glad we got this right in the second iteration. I will release a new hook version soon (will be a new git tag), as for now, your team can just use the hash you used for testing.

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.

None yet

2 participants