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

Make Options (De)serialisable with serde #70

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

byhemechi
Copy link

I've added basic serde attributes to the Config struct and its dependencies, only within the serde feature.

Please have a brief check that i've not missed any renames so that we stay in sync with micromark

Feel free to be rude if i've done anything stupid

All existing tests pass though I haven't written any new ones yet

@byhemechi
Copy link
Author

byhemechi commented May 23, 2023

please don't merge this yet, i'm yet to set up the defaults properly

it should be good now, dont merge without checking though of course

@byhemechi byhemechi closed this May 23, 2023
@byhemechi byhemechi reopened this May 23, 2023
@byhemechi
Copy link
Author

The check fail is on a file outside the scope of this PR so i'm not going to fix it here

@wooorm
Copy link
Owner

wooorm commented May 23, 2023

yeah don’t worry about that, that was my commit just now, will fix soon!

@wooorm
Copy link
Owner

wooorm commented May 25, 2023

Thanks! Code looks good, I do wonder if there should be some tests? And, should be documented in the features docs: https://docs.rs/markdown/1.0.0-alpha.9/markdown/#features

@byhemechi
Copy link
Author

I've adjusted the docs, will work on tests very soon. Are you ok with pulling in serde_json as a dev dependency for said tests?

@wooorm
Copy link
Owner

wooorm commented May 26, 2023

Sure!

@byhemechi
Copy link
Author

byhemechi commented May 26, 2023

Alright, doing tests now. To be entirely honest i don't know if these test actually achieve anything as all they are really doing is testing serde behaviour, which is already in their tests and is obviously covered by semver

@wooorm
Copy link
Owner

wooorm commented May 26, 2023

I guess you can check if it works, and that there is a specific string being generated?

@byhemechi
Copy link
Author

byhemechi commented May 26, 2023

yeah, that's what i'm ending up doing :)

testing deserialisation is mildly annoying though because nothing is Eq

@wooorm
Copy link
Owner

wooorm commented Jun 28, 2023

last I remember you were still working on this, right? Or is it done?

@byhemechi
Copy link
Author

byhemechi commented Jun 29, 2023

Sorry, got busy with life and didn't quite finish it :)

I've written some of the tests but once again they aren't really needed for coverage, serde has tests on the macros and the structs themselves already have tests

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