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

Optionally add nodoc #261

Merged
merged 5 commits into from
Jul 23, 2018
Merged

Optionally add nodoc #261

merged 5 commits into from
Jul 23, 2018

Conversation

Blacksmoke16
Copy link
Contributor

@Blacksmoke16 Blacksmoke16 commented Jul 21, 2018

This PR resolves #251.

It provides the ability to skip generation of Granite methods, macros, constants etc when using crystal docs. It will only not include the docs when the ENV var DISABLE_GRANITE_DOCS=true, if it is not set, or set to false it will include the Granite stuff.

The reasoning behind this; before this change on a just primary key model:

image

There is so much here that it almost makes the docs for the user's project unusable.

The same model skipping Granite stuff:

image

As you may notice this also adds the ability to define a comment to use on the getter/setter of each property with the following syntax:

field name : String, comment: "# The name of person"

or in this case:

primary id : Int64, comment: "# The primary key"

Oops, was on wrong branch


Final cleanup


Remove extra return


Allow comment on primary key
{% else %}
{{stmt.id}}
{% end %}
end
Copy link
Member

Choose a reason for hiding this comment

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

Clever. I like that the logic is centralized.

noDoc feels like a command though, which would prevent anything from being documented. Also it's snakeCased, which doesn't feel very Crystal.

Naming isn't my forte, but is there another name that would work better here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea...i'll think of something better

@@ -1,8 +1,18 @@
# Adds a :nodoc: to granite methods/constants if `DISABLE_GRANTE_DOCS` ENV var is true
macro noDoc(stmt)
{% if env("DISABLE_GRANTE_DOCS") == "true" %}
Copy link
Member

Choose a reason for hiding this comment

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

The purpose here is to make the docs more terse and relevant by default, right? Should this logic default to nodoc then?

Copy link
Contributor Author

@Blacksmoke16 Blacksmoke16 Jul 21, 2018

Choose a reason for hiding this comment

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

Depends on how you look at it. Was trying to allow you to disable the docs if you wanted while retaining current behavior.

But if we are okay with having them off by default i can do that.

Docs off by default
Fix typo
@Blacksmoke16
Copy link
Contributor Author

@robacarp how about disable_granite_docs?, now defaults to off unless the env variable is false

Copy link
Member

@robacarp robacarp left a comment

Choose a reason for hiding this comment

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

Thanks @Blacksmoke16

@robacarp robacarp merged commit 68b6f54 into amberframework:master Jul 23, 2018
@Blacksmoke16 Blacksmoke16 deleted the no-doc branch July 23, 2018 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add # :nodoc: to some properties/methods
2 participants