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 version macro & config deserialisation. #1601

Merged
merged 3 commits into from
Nov 25, 2024

Conversation

Jezza
Copy link
Contributor

@Jezza Jezza commented Nov 20, 2024

Just a couple of small fixes.

Nothing severe.

There's two changes here:

  1. re-export git_version, this lets us use it in the macro, meaning downstream crates don't have to pull in a whole new crate just for the macro.

  2. Fix number reading via ModeDependentValue.
    I don't honestly know how I was hitting this path, I suspect it has something to do with the arbitrary precision feature, but I'll admit that I didn't look too deeply into it.
    This change just accepts u64s as well, and then casts it to a signed number.

I can split it into two PRs if needed, but I thought I'd just cut down on the noise.

Copy link

PR missing one of the required labels: {'enhancement', 'new feature', 'internal', 'bug', 'breaking-change', 'dependencies', 'documentation'}

@Mallets Mallets added the internal Changes not included in the changelog label Nov 22, 2024
@Mallets
Copy link
Member

Mallets commented Nov 22, 2024

It seems the code format check is failing, make sure to run:

cargo fmt -- --config "unstable_features=true,imports_granularity=Crate,group_imports=StdExternalCrate"

Regarding point 2., wouldn't be that some integer number is interpreted directly as u64 instead of i64 or f64?
E.g., in the config you may declare field_f64: 4 and 4 is somehow interpreted as u64?

I'm just curious, besides that the PR LGTM and as soon as the CI is happy I'll merge it.

@Jezza
Copy link
Contributor Author

Jezza commented Nov 25, 2024

Thanks!

I pushed the formatter changes (Just a single newline).

Re: point 2.

I dived a bit deeper into it, and it seems that when serde_json parses a "0" when arbitrary_precision is disabled, it emits a U64.
image

Which after some thought, I now see why that differs.
Zenoh doesn't use serde_json, but json5.

@Mallets Mallets merged commit a75d2a1 into eclipse-zenoh:main Nov 25, 2024
13 checks passed
@Jezza Jezza deleted the jezza/fix branch November 27, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Changes not included in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants