Skip to content

Conversation

@thisisnic
Copy link
Member

@thisisnic thisisnic commented Jul 26, 2023

What changes are included in this PR?

Updates package version number to be character not numeric

Are these changes tested?

No, is configure scripts

Are there any user-facing changes?

No

@thisisnic thisisnic requested a review from assignUser as a code owner July 26, 2023 08:06
@github-actions github-actions bot added the awaiting committer review Awaiting committer review label Jul 26, 2023
@github-actions
Copy link

⚠️ GitHub issue #36883 has been automatically assigned in GitHub to PR creator.


# Small dev versions are added for R-only changes during CRAN submission.
if (is.na(dev_version) || dev_version < 100) {
if (is.na(dev_version) || dev_version < "100") {
Copy link
Member

Choose a reason for hiding this comment

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

Does the logic still work? gt/lt string comparisons can be funky iirc. Maybe we should explicitly convert it to a version object (or what ever it's called)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is what was recommended in the email from CRAN, but you're right, i.e. "10" > "2" == FALSE.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, nope, it doesn't matter; because dev_version is a package_version object, it's fine (i.e. in this case testing that a dev version of 10 is higher than the string "2" returns TRUE in the above comparison when done with the right classes)

Copy link
Member Author

Choose a reason for hiding this comment

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

x <- package_version("0.0.0.10")[1, 4]
x
#> [1] '10'
x > 2
#> [1] TRUE

Created on 2023-07-27 with reprex v2.0.2

Copy link
Member

Choose a reason for hiding this comment

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

Ah nice, yay for implicit casts :D

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jul 26, 2023
@thisisnic thisisnic requested a review from assignUser July 27, 2023 22:15
@thisisnic thisisnic merged commit 8c4941b into apache:main Jul 28, 2023
thisisnic added a commit to thisisnic/arrow that referenced this pull request Aug 23, 2023
…apache#36884)

### What changes are included in this PR?

Updates package version number to be character not numeric

### Are these changes tested?

No, is configure scripts

### Are there any user-facing changes?

No
* Closes: apache#36883

Authored-by: Nic Crane <thisisnic@gmail.com>
Signed-off-by: Nic Crane <thisisnic@gmail.com>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
…apache#36884)

### What changes are included in this PR?

Updates package version number to be character not numeric

### Are these changes tested?

No, is configure scripts

### Are there any user-facing changes?

No
* Closes: apache#36883

Authored-by: Nic Crane <thisisnic@gmail.com>
Signed-off-by: Nic Crane <thisisnic@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[R] Remove version number which triggers CRAN warning

2 participants