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

Replace instances of scorrect with typos #198

Merged
merged 1 commit into from
Jan 11, 2021
Merged

Replace instances of scorrect with typos #198

merged 1 commit into from
Jan 11, 2021

Conversation

johnmaguire
Copy link
Contributor

@johnmaguire johnmaguire commented Jan 11, 2021

At some point early on, this project was renamed from "scorrect" to
"typos". Some references to old URLs still exist. This commit corrects
those references.

Unrelatedly, I'm facing an issue building typos as one of the dependencies, derive_setters, fails to build after an update (v1.0.58) to its own dependency, syn changed its public API: Lymia/derive_setters#3

I attempted to raise an issue or revert PR with https://github.com/dtolnay/syn, as this is a breaking change in a point-release, but the project does not allow non-contributors to create issues or pull requests.

I then attempted to pin syn to "1.0.57" in this project's Cargo.toml, however it seems this does not affect what dependency version is selected for sub-dependencies (please, correct me if I'm mistaken.) It seems the only option is to wait for derive_setters to merge the previously referenced pull request.

In the meantime, I will attempt to build the project using instructions here: https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html

At some point early on, this project was renamed from "scorrect" to
"typos". Some references to old URLs still exist. This commit corrects
those references.
@epage
Copy link
Collaborator

epage commented Jan 11, 2021

I attempted to raise an issue or revert PR with https://github.com/dtolnay/syn, as this is a breaking change in a point-release, but the project does not allow non-contributors to create issues or pull requests.

Its ... debatable whether this was technically a breaking change, from what I understand. Rust evaluates macros in the caller's context which means

  • Macros cannot access your dependencies, only the dependencies the caller has set. So unless you force your caller to add a bunch of dependencies, you need to re-export them
  • Macros can only access pub items, so all of your re-exports need to be publically available.

There are conventions in the Rust community for "private by convention" to work around situations like this (other examples are enums or structs with pub fields and you don't want to allow exhaustive pattern matching, preventing you from adding new items). imo syn didn't do "enough" to follow the convention (like add a __ prefix to the module). It is easy to mistake code you read as public code. I'm a bit surprised because the maintainer is one of the most respected members of the community, creating some pretty amazing crates and pushing the boundaries of what people thought was possible.

I then attempted to pin syn to "1.0.57" in this project's Cargo.toml, however it seems this does not affect what dependency version is selected for sub-dependencies (please, correct me if I'm mistaken.) It seems the only option is to wait for derive_setters to merge the previously referenced pull request.

Yeah, I've seen this issue circulating around. You should be able to add

[dependencies]
syn = "=1.0.57"

to the top level crate and it should work. See https://users.rust-lang.org/t/syn-1-0-58-breaks-test-case-and-parameterized/53823

I just verified this by deleting my Cargo.lock, failing a build, and then editing Cargo.toml and the build worked.

I've gone ahead and created #199

@epage
Copy link
Collaborator

epage commented Jan 11, 2021

At some point early on, this project was renamed from "scorrect" to
"typos". Some references to old URLs still exist. This commit corrects
those references.

Sorry about that; that was a placeholder name while I came up with one. I'm surprised I missed things as I assume I would have searched until they were gone.

@epage epage merged commit 15147af into crate-ci:master Jan 11, 2021
@johnmaguire
Copy link
Contributor Author

I just verified this by deleting my Cargo.lock, failing a build, and then editing Cargo.toml and the build worked.

Duh, I forgot about Cargo.lock. In the meantime I built it using a [crates-io.patch] pointing to the pull request branch I linked above.

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.

2 participants