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 new (experimental) function write_license_note #271

Merged
merged 32 commits into from
May 11, 2023

Conversation

eitsupi
Copy link
Contributor

@eitsupi eitsupi commented May 1, 2023

Close #236

@eitsupi
Copy link
Contributor Author

eitsupi commented May 2, 2023

Now that the tests are working properly, I think you can merge this.

R/license_note.R Outdated Show resolved Hide resolved
R/license_note.R Outdated Show resolved Hide resolved
DESCRIPTION Outdated Show resolved Hide resolved
R/license_note.R Outdated Show resolved Hide resolved
@eitsupi

This comment was marked as outdated.

@eitsupi eitsupi marked this pull request as draft May 2, 2023 15:26
@eitsupi eitsupi marked this pull request as ready for review May 2, 2023 17:31
@eitsupi
Copy link
Contributor Author

eitsupi commented May 2, 2023

Replacement by cargo metadata failed, it seems that cargo license is still necessary.
However, by using cargo metadata, RcppTOML dependency could be removed.

R/license_note.R Outdated Show resolved Hide resolved
R/license_note.R Outdated Show resolved Hide resolved
@Ilia-Kosenkov
Copy link
Member

Will there be any benefit to storing license info in a tibble and then mutating it rather than working over disconnected lists?

@eitsupi
Copy link
Contributor Author

eitsupi commented May 4, 2023

Thanks for the review. I updated.

Will there be any benefit to storing license info in a tibble and then mutating it rather than working over disconnected lists?

cargo-license can also output data as tsv instead of json, but I think additional packages like readr or data.table would be needed to read it on the R side.
Here, I parse json and handle lists with the same structure as json, eliminating the need to depend on those packages.

R/license_note.R Outdated Show resolved Hide resolved
Copy link
Member

@Ilia-Kosenkov Ilia-Kosenkov left a comment

Choose a reason for hiding this comment

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

LGTM, a couple of wording improvements here and there.

R/license_note.R Outdated Show resolved Hide resolved
R/license_note.R Outdated Show resolved Hide resolved
R/license_note.R Outdated Show resolved Hide resolved
R/license_note.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
tests/testthat/helper.R Outdated Show resolved Hide resolved
eitsupi and others added 3 commits May 8, 2023 20:14
Co-authored-by: Ilia Kosenkov <ilia.kosenkov.at.gm@gmail.com>
Copy link
Member

@CGMossa CGMossa left a comment

Choose a reason for hiding this comment

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

LGTM as well.. I of courses waited until @Ilia-Kosenkov did all the work. But atleast i've skimmed it!

R/license_note.R Outdated Show resolved Hide resolved
Copy link
Contributor

@JosiahParry JosiahParry left a comment

Choose a reason for hiding this comment

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

Just a singular typo. Otherwise looks lovely.

JosiahParry

This comment was marked as duplicate.

@Ilia-Kosenkov Ilia-Kosenkov merged commit bff8028 into extendr:main May 11, 2023
17 checks passed
@eitsupi eitsupi deleted the license-note branch May 11, 2023 14:10
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.

A function or script to generate LICENSE.note
4 participants