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

use normalizePath() in find_package() #1765

Merged
merged 6 commits into from
Nov 9, 2022
Merged

Conversation

AshesITR
Copy link
Collaborator

@AshesITR AshesITR commented Nov 9, 2022

fixes #1759

Also fixed a (newly generated) warning

Warning (test-settings.R:108): it has a smart default for encodings
path[1]="dummy_packages/cp1252/R/metropolis-hastings-rho.R": No such file or directory
Backtrace:
 1. testthat::expect_identical(...)
      at test-settings.R:108:2
 6. lintr:::find_package(pkg_file)
 7. base::normalizePath(path)
      at lintr/R/settings_utils.R:7:2

Previously, existence of the path provided to lint_package() was never tested so it slipped by.

@codecov-commenter
Copy link

codecov-commenter commented Nov 9, 2022

Codecov Report

Merging #1765 (f08409d) into main (f486464) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1765   +/-   ##
=======================================
  Coverage   98.87%   98.87%           
=======================================
  Files         109      109           
  Lines        4622     4623    +1     
=======================================
+ Hits         4570     4571    +1     
  Misses         52       52           
Impacted Files Coverage Δ
R/settings_utils.R 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@AshesITR
Copy link
Collaborator Author

AshesITR commented Nov 9, 2022

@IndrajeetPatil there are currently 135 more undesirable_operator lints in this branch (ignoring the nolinted ones).
All of them lint a lintr::: expression in our test suite.
Maybe we should consider removing that one?

Note these are the only lints generated by withr::with_options(list(lintr.linter_file = ".lintr_new"), lint_package()) at the moment.

@IndrajeetPatil
Copy link
Collaborator

Maybe we should consider removing that one?

Definitely. We can add it back once #1692 is resolved.

Feel free to make the change in this PR.

@AshesITR
Copy link
Collaborator Author

AshesITR commented Nov 9, 2022

Okay. Let's do that in a follow-up to keep this PR focused.

Comment on lines 1 to 8
# This file is part of the standard setup for testthat.
# It is recommended that you do not modify it.
#
# Where should you do additional test configuration?
# Learn more about the roles of various files in:
# * https://r-pkgs.org/tests.html
# * https://testthat.r-lib.org/reference/test_package.html#special-files

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: We don't need to include this. It is unnecessarily distracting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed. We can always recreate this if we want to.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for the misunderstanding.

I meant that we don't need that comment in the testthat.R file. It would still be good to have the testthat.R file without those comments. Otherwise, it'd be confusing to have tests without nothing executing those tests.

IndrajeetPatil
IndrajeetPatil previously approved these changes Nov 9, 2022
Copy link
Collaborator

@IndrajeetPatil IndrajeetPatil left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Just one nitpick, but I am fine either way it is resolved.

IndrajeetPatil
IndrajeetPatil previously approved these changes Nov 9, 2022
@IndrajeetPatil IndrajeetPatil self-requested a review November 9, 2022 18:46
@IndrajeetPatil IndrajeetPatil merged commit 7a4fcee into main Nov 9, 2022
@IndrajeetPatil IndrajeetPatil deleted the fix/1759-find_package-dot branch November 9, 2022 19:40
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.

lint_package() and find_package(".") does not work anymore if depth >= 2
3 participants