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

Add a list of downstream repos, and script to update #1304

Merged
merged 63 commits into from
Jun 13, 2022
Merged

Add a list of downstream repos, and script to update #1304

merged 63 commits into from
Jun 13, 2022

Conversation

MichaelChirico
Copy link
Collaborator

@MichaelChirico MichaelChirico commented May 27, 2022

Related: #1292

revdep/get_urls.R Outdated Show resolved Hide resolved
revdep/get_urls.R Outdated Show resolved Hide resolved
revdep/get_urls.R Outdated Show resolved Hide resolved
@AshesITR
Copy link
Collaborator

I noticed .dev isn't covered by our lintr suite. Do we want to do something about it?

.dev/get_urls.R Outdated Show resolved Hide resolved
.dev/get_urls.R Outdated Show resolved Hide resolved
.dev/get_urls.R Outdated Show resolved Hide resolved
@MichaelChirico
Copy link
Collaborator Author

I guess we should also include a recommendation to add us as Suggests in the README? there's typically CRAN requirement all but it'll make things easier

@MichaelChirico
Copy link
Collaborator Author

I noticed .dev isn't covered by our lintr suite. Do we want to do something about it?

i think it's fine... not really production scripts, fine if they're a little rough around the edges

.dev/get_urls.R Outdated Show resolved Hide resolved
@MichaelChirico
Copy link
Collaborator Author

MichaelChirico commented Jun 5, 2022

@AshesITR PTAL. Added the e-mail templates -- these again will not be stored on GH (though the revdep-email-template file will be).

We can fine-tune the output a bit (or handle any bigger feedback), then I'll remove the temp artefacts & we can merge.

After that I'll put out a call for new untracked repos. I prefer if downstreams declare themselves to us, btw, rather than getting in downstreams' hair uninvited and/or dealing with low-quality/unmaintained repos. It's also a bit more of a security risk.

@AshesITR
Copy link
Collaborator

AshesITR commented Jun 6, 2022

I took a glimpse now.

  • Some maintainers might get multiple mails. Do we want to bundle all info in one mail?
  • The lint comparisons for brace_linter() and its predecessors always show up as a change, but I don't think this is material. Can we somehow exclude these sporadic hits?
  • Would it be worthwhile to dynamically list what is attached? e.g. if a failure is detected.
  • For the failures pertaining to a removed deprecated function, we could repeat the new linter to ease transition after an update to 3.0.0 has already been made. I see absolute_paths_linter() causing a few failures.

Also, maybe a good idea to run the checks with a soft-deprecated shim for the removed linters so we can check the remainder of the packages using now removed linters.
IINM we can achieve this by supplying the removed linter functions in globalenv() before calling lint_package().

@MichaelChirico
Copy link
Collaborator Author

Some maintainers might get multiple mails. Do we want to bundle all info in one mail?

Do you know what revdepcheck does here? i can see benefits of both ways. currently there are at most a few e-mails to be received so it may not be worth over-engineering...

The lint comparisons for brace_linter() and its predecessors always show up as a change, but I don't think this is material. Can we somehow exclude these sporadic hits?

Note that the differences are not matching on linter name, just on package/filename/line_number. So if brace_linter() is showing up, either the source marker has moved, or it's a new/removed lint. In any case, it should be surfaced to make sure it's intentional.

Would it be worthwhile to dynamically list what is attached? e.g. if a failure is detected.

Not following what this would look like...

For the failures pertaining to a removed deprecated function, we could repeat the new linter to ease transition after an update to 3.0.0 has already been made. I see absolute_paths_linter() causing a few failures.

I think there's been transition enough -- as noted in the NEWS, they've been marked as deprecated already since 2017. Time to pull the plug.

Also, maybe a good idea to run the checks with a soft-deprecated shim for the removed linters so we can check the remainder of the packages using now removed linters. IINM we can achieve this by supplying the removed linter functions in globalenv() before calling lint_package().

great idea 👍

@MichaelChirico
Copy link
Collaborator Author

OK @AshesITR cleaned up the PR here, should be ready for merge if there are no blockers

Copy link
Collaborator

@AshesITR AshesITR left a comment

Choose a reason for hiding this comment

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

Looks good so far, thanks!

.dev/revdep_compare_releases.R Outdated Show resolved Hide resolved
.dev/revdep_compare_releases.R Show resolved Hide resolved
.dev/revdep-email-template Outdated Show resolved Hide resolved
.dev/revdep-email-template Outdated Show resolved Hide resolved
.dev/revdep-email-template Outdated Show resolved Hide resolved
@MichaelChirico MichaelChirico requested a review from AshesITR June 13, 2022 07:45
@AshesITR AshesITR merged commit 3eb998e into main Jun 13, 2022
@AshesITR AshesITR deleted the rev-urls branch June 13, 2022 08:01
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.

3 participants