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

Added support for using sea-orm with #[deny(missing_docs)] #1522

Merged
merged 4 commits into from
Mar 10, 2023

Conversation

emirror-de
Copy link
Contributor

As discussed in #1472, this PR addresses the issue to allow #[deny(missing_docs)] in crates having sea-orm as dependency by adding #[doc = " Generated by sea-orm-macros"] to derived types/variants/fields in sea-orm-macros.

PR Info

New Features

  • Enables crates using sea-orm to use #[deny(missing_docs)].

Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

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

Hey @emirror-de, thanks for contributing!! Is this ready? Please click "Ready for review" once it's.

A nitpick: I wonder why there is a space before the documentation? #[doc = " Generated by sea-orm-macros"] Is there any reason behind it?

@emirror-de
Copy link
Contributor Author

Thanks for the review! I added it as draft to have a base for discussion and I was not sure if I missed something.

A nitpick: I wonder why there is a space before the documentation? #[doc = " Generated by sea-orm-macros"] Is there any reason behind it?

It is just a matter of style:

#[doc = " Generated by sea-orm-macros"] expands to /// Generated by sea-orm-macros while

#[doc = "generated by sea-orm-macros"] expands to ///generated by sea-orm-macros in source code.

The latter has been proposed in the corresponding issue. I personally prefer the first one, but I am happy to align it with the way it fits best for sea-orm.

I was also thinking in moving missing_docs from warn to deny in sea-orm and maybe add it to all the other crates that are currently missing it? But I am not sure if that is out of scope here?

@tyt2y3
Copy link
Member

tyt2y3 commented Mar 8, 2023

#[doc = " Generated by sea-orm-macros"] expands to /// Generated by sea-orm-macros while

Interesting observation. I agree.

@tyt2y3
Copy link
Member

tyt2y3 commented Mar 8, 2023

I was also thinking in moving missing_docs from warn to deny in sea-orm and maybe add it to all the other crates that are currently missing it? But I am not sure if that is out of scope here?

May be we can enable this in one of our tests or examples, so as to prove the point. And we will be able to catch it in our CI moving forward. But not changing it in sea-orm itself.

@emirror-de
Copy link
Contributor Author

May be we can enable this in one of our tests or examples, so as to prove the point. And we will be able to catch it in our CI moving forward. But not changing it in sea-orm itself.

OK, so you suggest adding a project to the examples with:

Cargo.toml

[dependencies]
sea-orm = { path = "../../", features = ["default"] }

lib.rs

//! Denying missing docs test.
#![deny(missing_docs)]

If I understood you correctly?

@tyt2y3
Copy link
Member

tyt2y3 commented Mar 9, 2023

Yes. Or we can simply hijack the basic example. It has no other external dependency (no web framework etc), so I think it will work.

@emirror-de emirror-de marked this pull request as ready for review March 9, 2023 13:40
@billy1624 billy1624 changed the base branch from master to pr/1522 March 10, 2023 10:49
Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

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

Hey @emirror-de, thanks for the CI update! I find there are more to be done. Adding #![deny(missing_docs)] alone won't catch it. We need to make the entity module as public in order to properly test it.

I'm going to take on the task :)

@billy1624 billy1624 merged commit 1305c85 into SeaQL:pr/1522 Mar 10, 2023
billy1624 added a commit that referenced this pull request Mar 10, 2023
* Added support for using sea-orm with #[deny(missing_docs)] (#1522)

* feat(macros): Added documentation tags for generated entities

* chore: Added deny(missing_docs) attribute to basic example

* chore: Fix clippy errors

* ci: test missing docs of derive macros generated types

* Try missing docs (CI should fail)

* Revert "Try missing docs (CI should fail)"

This reverts commit 83356bf.

---------

Co-authored-by: Lewin Probst, M.Sc <[email protected]>
@emirror-de emirror-de deleted the feature/1472-deny-missing-docs branch March 10, 2023 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

DeriveActiveEnum is incompatible with deny(missing_docs) attribute
3 participants