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.

Sign up for GitHub

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

Jump to bottom

chore: Clean version header parsing #610

Merged
merged 5 commits into from
Jun 8, 2023
Merged

chore: Clean version header parsing #610

merged 5 commits into from
Jun 8, 2023

Conversation

maelle
Copy link
Member

@maelle maelle commented Jan 30, 2023

No description provided.

maelle
maelle commented Jan 30, 2023
View reviewed changes
tests/testthat/_snaps/fledgling.md Outdated
@@ -12,7 +12,7 @@
},
"section_state": "keep",
"title": "fledge v2.0.0",
"version": "2.0.0",
"version": "v2.0.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

do we actually want to clean this?

Copy link
Member Author

Choose a reason for hiding this comment

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

@krlmlr what did you expect here, should the version stored by fledgeling keep the v or not?

Copy link
Contributor

Choose a reason for hiding this comment

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

No preference.

Copy link
Contributor

@krlmlr krlmlr Feb 3, 2023 •
edited

Choose a reason for hiding this comment

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

The fledgeling object should be opaque, the behavior of its API is more interesting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rethinking this. There's reason to leave the "v" out because then it can be easier converted to a numeric version.

base::numeric_version("v1.2.3")
#> Error: invalid version specification 'v1.2.3'
base::numeric_version("1.2.3")
#> [1] '1.2.3'

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

@maelle maelle requested a review from krlmlr January 30, 2023 12:02
@maelle maelle mentioned this pull request Jan 30, 2023
Merged
4 tasks
krlmlr
krlmlr reviewed Feb 1, 2023
View reviewed changes
Copy link
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks. PRs on top of PRs are difficult to reason about. Can we get parts of the "big" PR merged quickly?

tests/testthat/_snaps/parse-news-items/NEWS.md Outdated
@@ -1,6 +1,6 @@
<!-- NEWS.md is maintained by https://cynkra.github.io/fledge, do not edit -->

# fledge 0.0.1 (2023-01-23)
# fledge 0.0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes! otherwise it varies every day, it was a mistake from earlier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use a fixed date?

Base automatically changed from write-fledgeling to main February 4, 2023 13:57
This was referenced Feb 4, 2023
Closed
Closed
@krlmlr krlmlr marked this pull request as draft May 27, 2023 08:46
@krlmlr krlmlr changed the title clean version header parsing chore: Clean version header parsing May 27, 2023
krlmlr
krlmlr reviewed May 27, 2023
View reviewed changes
tests/testthat/_snaps/fledgling.md Outdated
@@ -12,7 +12,7 @@
},
"section_state": "keep",
"title": "fledge v2.0.0",
"version": "2.0.0",
"version": "v2.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Rethinking this. There's reason to leave the "v" out because then it can be easier converted to a numeric version.

base::numeric_version("v1.2.3")
#> Error: invalid version specification 'v1.2.3'
base::numeric_version("1.2.3")
#> [1] '1.2.3'

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

tests/testthat/_snaps/fledgling.md Outdated
@@ -26,7 +26,7 @@
},
"section_state": "keep",
"title": "fledge v1.0.0",
"version": "1.0.0",
"version": "v1.0.0",
"date": "NA",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just leaving those out, or setting them to an explicit NULL, would be more idiomatic? What are the upsides of storing NA here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, if we make them null, it makes this line fail

fledge/R/utils-parse-version.R

Line 3 in 40e192a

do.call(rbind, list)

krlmlr
krlmlr reviewed May 29, 2023
View reviewed changes
R/utils-parse-version.R Outdated Show resolved Hide resolved
@maelle maelle marked this pull request as ready for review May 29, 2023 06:31
@maelle maelle requested a review from krlmlr June 8, 2023 07:34
@krlmlr krlmlr merged commit c9c8d41 into main Jun 8, 2023
12 checks passed
@krlmlr krlmlr deleted the parse-version branch June 8, 2023 07:47
@krlmlr
Copy link
Contributor

krlmlr commented Jun 8, 2023

Thanks!

In the future, please don't let me block your progress in this repo. I'm happy to review "after the fact" as needed, or if you have specific questions regarding the code.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@krlmlr krlmlr Awaiting requested review from krlmlr

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@maelle @krlmlr