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

Expose parse_format_string to external users to allow for attempting to typecast raw data #5

Merged
merged 6 commits into from
Dec 11, 2023

Conversation

DaBs
Copy link
Contributor

@DaBs DaBs commented Nov 1, 2023

Heya, very neat to have this module!

We don't have the correct types initially for e.g. char types, as we're using this for compressed logging, so all of our "arguments" start out as u8 types, and we don't have the original type available for each argument. By exposing the format parts, we get to use them to attempt to create / typecast the different arguments to the correct types to then finally cast as &dyn Printf.

@tjol
Copy link
Owner

tjol commented Nov 1, 2023

I suppose I don't see the harm in making this public.

If the function is public it should probably have a doc comment

@DaBs
Copy link
Contributor Author

DaBs commented Nov 15, 2023

Sorry for the late reply - I've updated the PR with a doc comment now.
LMK if there's anything else to change.

@tjol
Copy link
Owner

tjol commented Dec 6, 2023

No need to apologize. Clearly, I'm worse.

(Sorry for leaving this PR hanging and not even approving the CI run, my private life has actually been quite hectic recently)

It looks like the doctest doesn't compile right now

@DaBs
Copy link
Contributor Author

DaBs commented Dec 11, 2023

All good - looks like I mis-formatted the doc comment. Should run correctly now.
On the subject of tests, wondering if it makes sense to add some for this as well now, seeing as it's a public facing API. But it's indirectly tested via the existing tests I suppose.

@tjol tjol merged commit 10ca0af into tjol:master Dec 11, 2023
1 check passed
@tjol
Copy link
Owner

tjol commented Feb 12, 2024

So I finally got around to releasing the change. (Hopeless, I know)

After reviewing the docs, I shifted things around a bit and decided to make the parser module public instead of pulling everything into the crate namespace. So be aware that there's a breaking API change between the recent git HEAD and releae 0.2.0.

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