Skip to content

Add compile-time flag to docs generator#6668

Merged
sdogruyol merged 3 commits into
crystal-lang:masterfrom
straight-shoota:jm/feature/docs-generator-options
Feb 15, 2019
Merged

Add compile-time flag to docs generator#6668
sdogruyol merged 3 commits into
crystal-lang:masterfrom
straight-shoota:jm/feature/docs-generator-options

Conversation

@straight-shoota
Copy link
Copy Markdown
Member

@straight-shoota straight-shoota commented Sep 5, 2018

This PR adds a docs flag when running the docs generator.

A macro like {% if flag?(:docs) %}{% end %} can be used to include some conditional code for the docs generator.

While it is generally not advised to customize code for generating the API docs, there are situations where this is required.

For example, the stdlib includes some features that are only available for specific targets and skipped on all other targets. For example FileDescriptor#fcntl is only available on POSIX, but WinError only on Windows. It is certainly not said that these will stay as they are and there should definitely be a strong motivation to try to minimize differences between platforms, especially in the stdlib. But some features simply won't work on all platforms. The docs generator is also not only used for stdlib API docs but for any Crystal library and they might have reasons for not having the API as much platform-independent as possible.

This PR just adds the compiler feature, that doesn't necessarily mean it should be used in the stdlib.

The second commit adds -D flag to crystal docs to allow specifying custom flags with the docs generator in the same way as when building. This is mostly for completeness sake, but can be useful to use the same flags for building and generating docs (for example -Dwithout_openssl should produce API docs without SSL; I know this is not a particularly convincing example, but that's the kind of thing this is about).

@straight-shoota straight-shoota changed the title Add compile-time flags to docs generator Add compile-time flag to docs generator Sep 5, 2018
@RX14
Copy link
Copy Markdown
Member

RX14 commented Sep 7, 2018

This is masking the problem, WinError should be hidden away in Crystal::System (and so should Errno but thats different) and #fcntl should exist but error on windows.

@straight-shoota
Copy link
Copy Markdown
Member Author

I know the examples are not particularly great and explicitly mentioned that. I also mentioned that this is not about using it in the stdlib, but making it available for other libraries as well, where there might be good reasons to provide target-specific APIs.

@ysbaddaden
Copy link
Copy Markdown
Collaborator

I'm fine with this, it could be useful. Thought this is hypothetical.

@asterite
Copy link
Copy Markdown
Member

asterite commented Sep 7, 2018

@straight-shoota I read the whole description and I don't understand what a docs flag has to do with fcntl or multiple platforms.

@straight-shoota
Copy link
Copy Markdown
Member Author

@asterite I'll use a more abstract example to not suggest anything for fcntl which could be misinterpreted:

The following code shows a type that should only exist on linux. Let's assume it provides some os-specific feature.

{% if flag?(:linux) %}
# This type does some linux things
class Foo
end
{% end %}

When compiling for linux, Foo can be used, on any other platform, it won't be available and raise a compile error. So far, this is fine.

But when generating API docs, there will be differences depending on platform: docs generated on linux will contain the type but it won't show up when generated on any other system.

That's bad because the API docs should better describe the entire API and not just that part that is randomly available on the system where the docs generator was run. This could be circumvented by always building docs on linux, but that will only go as far as there are no mutually exclusive restrictions.

With a docs flag, the compiler can be aware that it is currently processing code to generate API docs. This could be used to extend the condition in my example to flag?(:linux) || flag?(:docs) to always include the code when building docs. It shouldn't matter if it uses os specific system calls because there is no codegen.

@RX14
Copy link
Copy Markdown
Member

RX14 commented Sep 10, 2018

I still don't see this as a good idea. Types should be available across all platforms and incompatabilities should be runtime errors. This avoids moving the {% if flag? %} nonsense to the user's code.

@asterite
Copy link
Copy Markdown
Member

@RX14 Yes, I agree with you for the standard library. I think @straight-shoota is talking about shards that provide posix-only features, for example.

@straight-shoota
Copy link
Copy Markdown
Member Author

Exactly. Or, more precisely, shards that provide some platform specific types. If the entire shard only works on posix, it probably doesn't make much sense to generate the docs on windows. But that could still be possible.

Another use case for the stdlib that might be worth exploring could be to get rid of docs_main.cr. Not sure if that would work out, though.

@RX14
Copy link
Copy Markdown
Member

RX14 commented Sep 10, 2018

@asterite if it makes sense in the stdlib, why not for shards?

@straight-shoota
Copy link
Copy Markdown
Member Author

@RX14 How else would you use platform-specific features?

I don't think it makes any sense to be able to build for example a Linux executable accessing a stubbed Windows Registry API and just fail at runtime. This shouldn't even compile in the first place.

It's fine to stub methods raising runtime errors if they're part of a common, platform-independent API and only specific parts are not cross-platform.
But when entire types are not available on some platforms, it should be dealt with in user code to never use such types on the non-working platforms.

This is certainly opinionated and other approaches are equally valid. But developers should have a choice to use platform-specific APIs and hide them on other systems.

@straight-shoota
Copy link
Copy Markdown
Member Author

But honestly, I really don't understand why there is so much trouble about this.

The compiler supports compile time flags. It only makes sense that the docs generator also supports them. That's currently missing and this PR adds that.

I haven't heard any argument against that.

Now, this PR also adds docs as default flag when running the docs generator. This is maybe not strictly necessary and it could always be emulated by calling crystal docs -Ddocs (assuming the main part of this PR is merged).

I think it is a nice feature to have this flag set by default. Crystal source code can be manipulated by macros and the available APIs can depend on the target triple or other compiler flags. If the available types, methods etc. are configurable at compile time, there should be a way to determine if the compiler is generating docs to be able to document all APIs and not just the ones available when compiling for the current platform.

API docs should always be the same wherever they are built, but the available APIs don't necessarily. As long as the language allows to have platform-dependent APIs, there is no sense in discussing if the docs generator should support building combined docs for this.

@RX14
Copy link
Copy Markdown
Member

RX14 commented Sep 10, 2018

@straight-shoota no - it absolutely should compile even if the whole type or API isn't cross platform. You will eventually want or need to put a RegistryNode in some generic cross-platform class in your project then you'll need to write {% if flag? %} everywhere and that's ugly. Even for just one class you need to ifdef the accessor, ifdef the constructor and if you have a custom equals, hash, they have to get ifdef.

Whereas if you stub out he type on unix and simply make every method raise it's a lot easier to integrate into your project at large.

And they still have the choice to make it hard for users, they will just be missing one tiny feature from the doc generator.

@RX14
Copy link
Copy Markdown
Member

RX14 commented Sep 10, 2018

I'm in favour of allowing -D to be passed to the docs generator, that definitely does have a usecase. I didn't realise this PR did that too.

In fact the docs generator should support all the non-codegen compiler flags including --prelude=empty (I have a friend who needs this to document their custom crystal stdlib for the nintendo switch).

If you switch the doc generator to using create_compiler that would be fantastic.

@straight-shoota
Copy link
Copy Markdown
Member Author

@RX14 Reusing create_compiler is not possible as it currently stands, because some of it's logic doesn't fit for the docs generator and it's all deeply integrated.
So I added a commit which duplicates the relevant compiler options. They're trivially implemented and a little bit duplication isn't that bad.

Maybe we could consider refactoring the entire arguments parsing at some point. But I'm not sure if if could make a significant improvement because of the many options, partly interconnected. And the arguments are supposed to be printed in a meaningful order with --help.

Copy link
Copy Markdown
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

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

I'm ok with the default docs flags. Note that the workaround only when adding functionality. Because methods can be overwritten and then certain conditions might hide some implementations.

@straight-shoota
Copy link
Copy Markdown
Member Author

Looks like this can be merged .

@@ -54,6 +85,7 @@ class Crystal::Command
included_dirs << File.expand_path("./src")

compiler = Compiler.new
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this instantiation of compiler needed? I mean, it is already declarated on line 14

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh, thanks for noticing. That's actually an error, this line needs to go.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This ought to go... 🏓

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Damnit, I overlooked that. Sorry.

Copy link
Copy Markdown
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Just need a rebase, thank you @straight-shoota

@straight-shoota straight-shoota force-pushed the jm/feature/docs-generator-options branch from 7fb41fe to 033eb58 Compare February 10, 2019 13:45
@straight-shoota
Copy link
Copy Markdown
Member Author

Rebased and ready to ship.

@sdogruyol sdogruyol added this to the 0.28.0 milestone Feb 15, 2019
@sdogruyol sdogruyol merged commit df0fdd2 into crystal-lang:master Feb 15, 2019
@straight-shoota straight-shoota deleted the jm/feature/docs-generator-options branch February 15, 2019 22: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.

8 participants