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

Fix logos_path in generated code #348

Merged
merged 1 commit into from
Nov 6, 2023
Merged

Conversation

jannik4
Copy link
Contributor

@jannik4 jannik4 commented Nov 3, 2023

No description provided.

@jeertmans
Copy link
Collaborator

Nice! Could you add a small test that would break if your fix was not present?

@jannik4
Copy link
Contributor Author

jannik4 commented Nov 3, 2023

I've already considered it, but unfortunately I don't think it's that easy.
The problem is that you need a crate that has access to logos without having logos directly as a dependency. You can't even shadow logos since the derive macro could (and should) use ::logos.

For example:

  • crate logos_reexport depends on logos and reexports logos
  • crate logos_use depends only on logos_reexport and uses logos_reexport::logos as logos_path

That should make sure it does not break again, but it adds two new crates to the repository just for this. If that's okay, please tell me where to put them and I can do that.

(Maybe there's something I'm missing, but I think this is the only way.)

@jeertmans
Copy link
Collaborator

Hello @jannik4, sorry for the late reply, but I needed to think a bit about this :-)

So I don't think we really need to create a new crate, depending on how cargo actually resolves dependencies.

I have two ideas:

  1. Try to create a submodule mod mylogos and re-export logos inside it:
    // any tests/tests/*.rs file, or actually could also be inside some #[cfg(test)]
    mod mylogos {
      pub use logos as logos_path;
    }
    or you can also create a custom module in the tests directory.
  2. Because the parent tests directory is actually a crate, you could actually re-export logos in tests/src/lib.rs.

The only doubt I have is whether the tests will fail without your fix, because cargo may be smart enough to see that your re-export points to the same logos as ::logos.

What do you think?

@jannik4
Copy link
Contributor Author

jannik4 commented Nov 6, 2023

Isn't this exactly the same as the already existing test tests/tests/crate_.rs? The problem here is that ::logos is always accessible, so if the derive is missing the custom path at one point it continues to just work.

@jeertmans
Copy link
Collaborator

Hum ok I guess this isn't as easy as I would have hoped, but not problem I will merge this like this then ;-) Thanks for your work!

@jeertmans jeertmans merged commit 3abd88b into maciejhirsz:master Nov 6, 2023
16 checks passed
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