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

Port docs and some other fixes #12

Merged
merged 9 commits into from
Apr 15, 2024
Merged

Port docs and some other fixes #12

merged 9 commits into from
Apr 15, 2024

Conversation

pitdicker
Copy link
Contributor

This ports the documentation and two simple commits from zoneinfo-parse.
I also included a few commits to fix rust and clippy warnings and to update to rust 2021.

@pitdicker
Copy link
Contributor Author

pitdicker commented Apr 14, 2024

Added one new commit.

We are currently unable to parse comments correctly in Zone lines.
The cause is that the until column can have an optional year, month, day and time. The regex matches any non-whitespace value as a year, month etc. Then it can be followed by an optional comment.

If a comment is present and not all four year, month, day and time values are present, the regex will consider the comment to be one of the four fields. This is easy to avoid by making the regex more precise.

This will allow us to remove a workaround in chrono-tz: https://github.com/chronotope/chrono-tz/blob/main/chrono-tz-build/src/lib.rs#L24.

@pitdicker
Copy link
Contributor Author

pitdicker commented Apr 15, 2024

Added the original history with a merge commit. Not sure if GitHub has a better way to visualize it than https://github.com/chronotope/parse-zoneinfo/commits/port_docs/.

@djc is this what you had in mind with:

  • Merge the git history from zoneinfo_parse into this repository somehow (probably by just doing an unrelated history merge)

    • We should make sure that the current commits are still part of the new main branch's commit ancestry

@djc
Copy link
Member

djc commented Apr 15, 2024

I think any new commits from you should be in a separate PR. For this PR I'd like to see a detailed list of the commands that you issued/work that you did to get here? Ideally this should be the minimal steps that go from git merge --allow-unrelated-histories https://github.com/rust-datetime/zoneinfo-parse master to something that will pass our current CI setup, keeping as much as possible from the other branch (doc comments and tests).

@pitdicker
Copy link
Contributor Author

In that case I'd rather make the merge in another PR, it was tacked on to the work here.
My primitive steps were to copy all files to a second directory, do git merge --allow-unrelated-histories, delete the files and copy back to resolve the merge conflicts.

@djc
Copy link
Member

djc commented Apr 15, 2024

I think we should merge the other repo's history before we add any additional changes here, otherwise the merge will only get more complicated.

@pitdicker pitdicker mentioned this pull request Apr 15, 2024
@pitdicker pitdicker force-pushed the port_docs branch 3 times, most recently from e2d124b to 12e87c6 Compare April 15, 2024 14:15
Cargo.toml Outdated Show resolved Hide resolved
src/line.rs Outdated
Comment on lines 972 to 974
pub fn new() -> Self {
Self::default()
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to just leave this out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not want to make this a breaking change only to fix a clippy warning. There are no other changes yet that warrant a new major release.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, maybe add a #[deprecated] at least?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I still prefer new() myself.

Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is shorter, familiar, and still somewhat of a convention to have new. Of course it is completely redundant if you have default().

Copy link
Member

Choose a reason for hiding this comment

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

Fair -- my counter arguments would be:

  • Short: 3 vs 7 characters, not an important difference
  • Familiar: here, too, the difference seems very small
  • Convention: both are a convention, but there is more type system pressure for Default

In the end, given that it is redundant, I prefer having less code.

src/line.rs Outdated Show resolved Hide resolved
@pitdicker pitdicker merged commit 0207cf4 into master Apr 15, 2024
2 checks passed
@pitdicker pitdicker deleted the port_docs branch April 15, 2024 14:49
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