Skip to content

Add CI with linting, formatting and testing (including docs)#6

Merged
valsteen merged 14 commits into
valsteen:mainfrom
tlaferriere:ci-1
Jun 20, 2023
Merged

Add CI with linting, formatting and testing (including docs)#6
valsteen merged 14 commits into
valsteen:mainfrom
tlaferriere:ci-1

Conversation

@tlaferriere

Copy link
Copy Markdown
Contributor

I had to rework the examples in the README.md in order to pass the doc tests. They are now completely functional and copy-pastable, except the EnumAccessor alias. Maybe my understanding of this feature is incomplete.

@tlaferriere

Copy link
Copy Markdown
Contributor Author

I removed the macro expansions, as they would be too hard to maintain and they are an implementation detail.

@tlaferriere

Copy link
Copy Markdown
Contributor Author

I added a drive-by fix for the lifetime test failure, let me know if it is appropriate.

@valsteen

Copy link
Copy Markdown
Owner

Hi, thanks for fixing the test, I think I rushed a bit the commit 😵‍💫

I'll have a better look at the example snippets and see if I can clarify EnumAccessor.

Also I know some Rust docs have runnable snippets that hide the boilerplate ( ex : https://rust-lang.github.io/async-book/02_execution/03_wakeups.html#applied-build-a-timer ), but maybe it's not available to snippets added to a README.md

@tlaferriere

Copy link
Copy Markdown
Contributor Author

I'll have a better look at the example snippets and see if I can clarify EnumAccessor.

I created a draft PR (#8) to differentiate accessors based on aliases also (I just modified the condition for emitting the duplicate accessor error to include the alias). This didn't break any existing tests, but I feel like there are some edge cases that could break it. I'll try to add more tests for such edge cases before merging this.

Also I know some Rust docs have runnable snippets that hide the boilerplate ( ex : https://rust-lang.github.io/async-book/02_execution/03_wakeups.html#applied-build-a-timer ), but maybe it's not available to snippets added to a README.md

This is a rustdoc feature so I'm pretty sure that GitHub would render these lines with the # at the start of the line, which would break syntax highlighting and look pretty ugly. I'll give it a try on my branch to see how it renders on Github.
Alternatively, we could keep only the simplest examples where it makes sense to keep the use statements in the README, and bringing all the more boilerplate-rich examples into the crate docs directly.

@valsteen

Copy link
Copy Markdown
Owner

Alternatively, we could keep only the simplest examples where it makes sense to keep the use statements in the README, and bringing all the more boilerplate-rich examples into the crate docs directly.

you're right, the current state of README is some braindump to compensate ... that there is no other doc. Hopefully documentation and runnable examples can be directly be attached to proc_macro_derive declarations.

I have to review my tests and make sure it cover wells the intended feature. 9 months ago already I hope I'll remember what I tried to do 😵‍💫

@tlaferriere

Copy link
Copy Markdown
Contributor Author

The syntax highlighting isn't broken, but this isn't copy-pasteable as compilable code from github anymore:
image

you're right, the current state of README is some braindump to compensate ... that there is no other doc. Hopefully documentation and runnable examples can be directly be attached to proc_macro_derive declarations.

I'll move the more complicated examples into the crate docs and attach them to the right macros. They map pretty well to the API, so it is going to make pretty good crate docs.

@tlaferriere

Copy link
Copy Markdown
Contributor Author

I added some badges 😉
image

@tlaferriere

Copy link
Copy Markdown
Contributor Author

Alright, now it looks and is professional.

@valsteen

Copy link
Copy Markdown
Owner

woa thanks I added a bit more coverage with some more (lame) smoke tests – at least there is a good branch coverage except for error cases.

I'll merge this to see those shiny badges

@valsteen
valsteen merged commit 7c6eb7f into valsteen:main Jun 20, 2023
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