Skip to content
This repository has been archived by the owner on Jul 7, 2023. It is now read-only.

Remove byteorder #15

Closed

Conversation

workingjubilee
Copy link
Contributor

@workingjubilee workingjubilee commented May 23, 2021

This PR removes byteorder, formally raises the MSRV to 1.40.0 (according to a run of cargo msrv), and adds a rust-toolchain file in order to control MSRV drift.

Closes #12.

Copy link
Owner

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

Thanks so much!

So normally, I'd be happy to take the Rust 2018 edition changes, but I would like to keep the diff here as small as possible to minimize conflicts with my ongoing rewrite branch. So to that end, could you please:

  • Drop the Rust 2018 migration commit.
  • For the byteorder removal change, minimize the changes such that the code still uses the full endian name, e.g., NativeEndian instead of NE.

And I think that will make the diff here pretty small.

@BurntSushi
Copy link
Owner

(Note that the rewrite already migrates to Rust 2018.)

@workingjubilee
Copy link
Contributor Author

Sounds good!

@workingjubilee
Copy link
Contributor Author

The last commit now just notes what was already functionally declared by the ci change and includes the toolchain file.

@@ -0,0 +1,2 @@
[toolchain]
channel = "1.40.0"
Copy link
Owner

Choose a reason for hiding this comment

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

Hmmm... I've never used a rust-toolchain file. Did you mean rust-toolchain.toml? In any case, I don't think that does what I want. I don't want to build and develop on this version, I just want to have it be supported. I think CI is good enough for that. Otherwise, I want to use whatever my default toolchain is. (Usually latest nightly.)

BurntSushi pushed a commit that referenced this pull request Jun 1, 2021
This takes #15 and shrinks its diff considerably. Namely, we match the
byteorder API a bit more exactly, remove all changes to
dense/sparse/state_id and keep the minimum things needed in lib.rs and
Cargo.toml.

While improvements are in general welcome, all of this machinery is gone
in the rewrite. The only point of this change is to satisfy a request to
remove a dependency, of which I am sympathetic to.

Closes #15
BurntSushi pushed a commit that referenced this pull request Jun 1, 2021
This takes #15 and shrinks its diff considerably. Namely, we match the
byteorder API a bit more exactly, remove all changes to
dense/sparse/state_id and keep the minimum things needed in lib.rs and
Cargo.toml.

While improvements are in general welcome, all of this machinery is gone
in the rewrite. The only point of this change is to satisfy a request to
remove a dependency, of which I am sympathetic to. Minimizing the diff
is important for sanely rebasing my rewrite branch on top of this.
Otherwise, removing this dependency probably becomes more annoying than
it's worth.

Closes #15
@BurntSushi BurntSushi mentioned this pull request Jun 1, 2021
@BurntSushi
Copy link
Owner

I opened #16 to supplant this one. Mostly I just really wanted to get the diff down to as small as possible. (Or at least, localized to a new file that I know I can just drop when I rebase my rewrite on top of this.)

BurntSushi pushed a commit that referenced this pull request Jun 1, 2021
This takes #15 and shrinks its diff considerably. Namely, we match the
byteorder API a bit more exactly, remove all changes to
dense/sparse/state_id and keep the minimum things needed in lib.rs and
Cargo.toml.

While improvements are in general welcome, all of this machinery is gone
in the rewrite. The only point of this change is to satisfy a request to
remove a dependency, of which I am sympathetic to. Minimizing the diff
is important for sanely rebasing my rewrite branch on top of this.
Otherwise, removing this dependency probably becomes more annoying than
it's worth.

Closes #15
BurntSushi pushed a commit that referenced this pull request Jun 1, 2021
This takes #15 and shrinks its diff considerably. Namely, we match the
byteorder API a bit more exactly, remove all changes to
dense/sparse/state_id and keep the minimum things needed in lib.rs and
Cargo.toml.

While improvements are in general welcome, all of this machinery is gone
in the rewrite. The only point of this change is to satisfy a request to
remove a dependency, of which I am sympathetic to. Minimizing the diff
is important for sanely rebasing my rewrite branch on top of this.
Otherwise, removing this dependency probably becomes more annoying than
it's worth.

Closes #15
@BurntSushi BurntSushi closed this in 306e14a Jun 1, 2021
@workingjubilee
Copy link
Contributor Author

ahhh, sorry! I misunderstood.

BurntSushi added a commit that referenced this pull request Jun 7, 2021
It was introduced in #15 and #16 to remove byteorder in the 0.1.x
series, but we had already migrated off that a while back in this branch.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minimizing dependencies
2 participants