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 filter function to @autodocs #885

Merged
merged 14 commits into from
Dec 5, 2018

Conversation

timziebart
Copy link
Contributor

@timziebart timziebart commented Nov 8, 2018

Hi, I have a simple implementation of adding a filter function to @autodocs via a Filter keyword (as shortly discussed in slack here and the corresponding thread). See here for an example index.md where the filtering function is created here. Do you generally agree with that idea?

It seems to work if I try it with this example setup https://github.com/timkittel/Documenter-test but I don't really know how I would add the tests here. Could you give me some pointers?

Of course, I would add a sction to the documentation if there is positive feedback here (:

@mortenpi mortenpi added this to the 0.21.0 milestone Nov 10, 2018
@mortenpi
Copy link
Member

Awesome, thanks! I don't think this really needs to be more complicated than this, so the implementation looks good.

Just needs docs as you mentioned. I am not entirely sure what are the types of the objects that will get passed to the filter function though -- it would be good to document that. For functions you probably get these Binding objects I assume.

Does it also work if you create a lambda function directly in the at-autodocs block?

For tests, I think we should add something to test/examples/.

@timziebart
Copy link
Contributor Author

Great!

Concerning lambda functions: No, unfortunately not. Providing a lambda function raises one of these world age errors, that I have never really understood. I assume there is somewhere a @generated function involved, but that would require to dig deeper into Documenter.jl.

I will add a description to the docs and my example to the tests. But that will take me a moment as I don't have access to a real computer at the moment (:

@fredrikekre
Copy link
Member

Providing a lambda function raises one of these world age errors, that I have never really understood.

Probably just need to invoke it with invokelatest at the callsite.

src/Expanders.jl Outdated Show resolved Hide resolved
@timziebart
Copy link
Contributor Author

@fredrikekre Thanks, I will try when updating the PR. Would be great if lambda functions worked!

@timziebart
Copy link
Contributor Author

timziebart commented Nov 30, 2018

@mortenpi , I updated the PR with lambda functions now working, an example in tests and a note in the documentation. Could you check whether you find it okai?

Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

Looks awesome, thanks!

Would you mind adding a brief description of this feature to CHANGELOG.md, under a new "Version v0.21.0" section? Scratch that -- I added it in #896. But wording suggestions welcome.

@@ -85,6 +85,27 @@ docstrings. Note that page matching is done using the end of the provided string
`a.jl` will be matched by *any* source file that ends in `a.jl`, i.e. `src/a.jl` or
`src/foo/a.jl`.

To filter out certain docstrings by your own criteria, you can provide function with them
Copy link
Member

Choose a reason for hiding this comment

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

s/them/the ?

mortenpi added a commit that referenced this pull request Dec 1, 2018
@mortenpi
Copy link
Member

mortenpi commented Dec 1, 2018

Hmm, I started wondering about what gets passed as the argument to the lambda function. E.g. if you add the following into the Filter module

"foobar"
function foobar end

you get this error

ERROR: LoadError: TypeError: in <:, expected Type, got typeof(Main.AutoDocs.Filter.foobar)

Could we have a short docs section on what are the types that will be used to call the filtering function and how to handle them?

Also, I was hoping that Order = [:type] would make the error go away, but it seems that the filtering does not work together with Order? Or Order is being done after Filter?

mortenpi added a commit that referenced this pull request Dec 2, 2018
@timziebart
Copy link
Contributor Author

timziebart commented Dec 3, 2018

Hmm, I started wondering about what gets passed as the argument to the lambda function. E.g. if you add the following into the Filter module

"foobar"
function foobar end

you get this error

ERROR: LoadError: TypeError: in <:, expected Type, got typeof(Main.AutoDocs.Filter.foobar)

Could we have a short docs section on what are the types that will be used to call the filtering function and how to handle them?

As far as I understand, Documenter.jl simply goes through the whole module and checks for everything that has a doc string inside. The filter is just run on everything, that Documenter.jl provides, so your case, you try to check whether the function is a sub type of an abstract type and that fails.
To avoid misunderstandings, I added a check for the type of the provided element to the filter in the docs: 6e70981#diff-1842511d55effdf48dacc8c5de700712R94 (and fixed my typo afterwards in c8cbb6f ^^ )

And the description in the CHANGELOG.md sounds very good to me, thanks!

Also, I was hoping that Order = [:type] would make the error go away, but it seems that the filtering does not work together with Order? Or Order is being done after Filter?

When trying that, I am stumbling over a an error that seems to be due to the parser of the @autodocs block. I will look into that and tell you when I know more ...

@timziebart
Copy link
Contributor Author

timziebart commented Dec 3, 2018

@mortenpi I modified the code so the Filter keyword is evaluated after the Order keyword. Now, providing Order = [:type] ensures that only types are given to the filter function. It is tested in the docs now as well, see 8cd62b6

That should conclude the PR?

On a side note: I would suggest to refactor the evaluation of the keywords a bit in order to avoid the for loops and if clauses which made it a bit hard for me to find the error. But that would be separate PR, I think. Still, would you generally be open for that, @mortenpi ? Then I would make a suggestion.

@mortenpi
Copy link
Member

mortenpi commented Dec 5, 2018

Yes, this is good to go. Thanks for iterating on this!

I would suggest to refactor the evaluation of the keywords a bit in order to avoid the for loops and if clauses which made it a bit hard for me to find the error. But that would be separate PR, I think.

That sounds good to me. If you'd be willing to make a PR, that would be awesome!

@mortenpi mortenpi merged commit a4079c7 into JuliaDocs:master Dec 5, 2018
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.

3 participants