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

improve defmt version mismatch diagnostics #242

Merged
merged 1 commit into from Nov 13, 2020
Merged

Conversation

japaric
Copy link
Member

@japaric japaric commented Nov 12, 2020

offer a different suggestion based on whether a git or crates.io version is being used

should be merged after #244

offer a different suggestion based on whether a git or crates.io version is being used
defmt = {{ git = \"https://github.com/knurling-rs/defmt\", rev = \"{0}\" }}
defmt-rtt = {{ git = \"https://github.com/knurling-rs/defmt\", rev = \"{0}\" }}
# ONLY pin this dependency if you are using the `print-defmt` feature
panic-probe = {{ git = \"https://github.com/knurling-rs/defmt\", features = [\"print-defmt\"], rev = \"{0}\" }}",
Copy link
Member Author

@japaric japaric Nov 12, 2020

Choose a reason for hiding this comment

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

panic-probe currently lives in the probe-run repo so it's not really possible to pin it to the correct defmt rev (panic-probe also already pins to a particular defmt and that cannot be changed from an application project). I think that to simplify the use of the git-version of defmt we'll have to move panic-probe (back) into the defmt repo. This PR is blocked on that migration.

Copy link
Member Author

Choose a reason for hiding this comment

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

migration PR: #244

@japaric japaric marked this pull request as ready for review November 12, 2020 15:06
Git,
}

impl Kind {
Copy link
Contributor

Choose a reason for hiding this comment

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

question: why are these defined within the function instead of outside?

Copy link
Member Author

Choose a reason for hiding this comment

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

the enum is only used within the function body so putting it inside avoids adding yet another type to the outer (src/lib.rs) type namespace.

Another way to keep the namespace tidy is use modules so this could have been written as: mod version { enum Kind { .. } pub fn check(version: &str) -> Result<(), String> { .. } }. We have not used modules in this crate at all so the file is over 1.6K lines ... we should eventually come back and refactor this into modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

alrighty, thank you for the clarification! +1 to the refactoring, but that's for another day ofc

@japaric
Copy link
Member Author

japaric commented Nov 12, 2020

reminder to self: publish a new patch version of defmt-decoder after this is merged

@japaric japaric merged commit 1a9361c into main Nov 13, 2020
@japaric japaric deleted the mismatch-diagnostics branch November 13, 2020 09:52
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.

None yet

2 participants