Skip to content
This repository has been archived by the owner on Jan 2, 2023. It is now read-only.

Support for splitting {{autogenerated}} tags to allow modular template files #89

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

moritzmeister
Copy link

Hi!

I really like this "plugin", first of all because it formats code signatures nicely and because of its simplicity.

I was looking for a way to be able to split up the {{autogenerated}} tag, in order to allow for additional content in between API documentation. However, I couldn't find a way. I made some minor changes to accomodate for this use case and this is a draft.

My intention was not to break your way of using it. However, the way I am doing it right now for the old case with a single {{autogenerated}} tag is not optimal, as I am reading and writing the file multiple times. The tag with the double curly brackets is not optimal from a python perspective as it as a special meaning in python string. I saw other plugins using tags like ![mkapi](<package.module.object>)

Basically, the identifier of the object to be documented becomes the {{tag}} in the template markdown file, a markdown file now might look like this:

## The XYZ package

{{xyz.def}}

Some extra content here.

!!! example
    ```python
    import xyz
    ```
  
{{xyz.asd}}

If you like this feature, I am happy to continue working on it and make it compliant to your way of doing things :)
That's why I also haven't added tests for the changes I made yet.

Let me know what you think!

@gabrieldemarmiesse
Copy link
Contributor

Thank you for the pull request! It's true that this lib lacked a more powerful way of declaring templates, it's long overdue

I Really like your idea of putting arbitrary identifiers :) I'm not so fond of putting the name of the object the user wants to insert as ID. With this lib, we really aim at flexibility from the user side. And the user might want to declare multiple object in the same tag. For example it's not compatible with the functions get_methods or get_functions.

What I would propose, if you're ready to implement it is to allow arbitrary strings to be IDs in the template. To refer to them in the autogen.py, we need a way to specify the tag ID and the file name as the dict key. Something like that seems flexible and intuitive:

# This is a markdown template

{{pretty_tag}}

some text

{{another_pretty_tag_name}}
#autogen.py 

pages = {
    ("./templates/my_file.md", "pretty_tag") : get_methods("my_object"),
    ("./templates/my_file.md", "another_pretty_tag_name") : "some_module.some_function_to_document",
}

By default, if a tuple is not used as the key, we assume that we use the old behavior (no template or {{autogenerated}} tag).

In this way we would be backward compatible, flexible and intuitive. Would you be ready to implement it? Or if you want to challenge this proposal and make another one, feel free :)

@moritzmeister
Copy link
Author

Glad you think it's a nice addition to make the tags more flexible. And I agree with you that my proposal is not the way to do it, I didn't fully think it through, just wanted to get my idea across.

I am happy to work with you on making this proper. Will have to do it on the weekend though.

Your proposal makes sense with the possibility to define "pretty tags", just from an API perspective to make it a bit easier for the user, e.g. less typing, I would propose moving the tag from the tuple to the objects to be documented, so the user doesn't have to duplicate the template path for every tag within the same template:

#autogen.py 

pages = {
    "./templates/my_file.md": {
        "pretty_tag": get_methods("my_object"),
        "another_pretty_tag_name": "some_module.some_function_to_document"
    },
    "./templates/other_file.md": get_methods("my_other_object")
}

This way the user has to group together content also in the definition which belongs to the same template. If no dictionary is provided as value, instead a list or single value we can default to the old behaviour. Also it avoids situations where a user specifies a custom tag for a certain template, but in another line defines the same template without custom tag:

# undesired behaviour

pages = {
    ("./templates/my_file.md", "pretty_tag") : get_methods("my_object"),
    ("./templates/my_other_file.md", "pretty_tag") : get_methods("my_other_object"),
    ("./templates/my_other_file.md", "pretty_tag") : get_methods("my_other_object"),
    "./templates/my_file.md": "some_module.some_function_to_document",
}

What do you think?

@gabrieldemarmiesse
Copy link
Contributor

I like your proposal which uses dicts as values, it make the core more readable by grouping things together :) if you'd be so kind as to implement it and add tests, I'll gladly merge it

@moritzmeister
Copy link
Author

moritzmeister commented Oct 21, 2020

Hey @gabrieldemarmiesse,

sorry for the long wait. I finally got around implementing this :)

My last commit reverts the previous implementation and updates it according to the desired functionality, that we discussed.
It is still missing tests for the added functionality, but since we wanted to keep the old behaviour unchanged, the existing tests should not fail.

Just wanted you to have a look at the implementation first, to avoid having to rewrite tests, I will add tests for the added if-branches, once you reviewed the implementation :)

Looking forward to your review.

Update: Just noticed that GitHub shows some changes which I seem to have pulled in from master. Don't let them confuse you :)

Copy link
Contributor

@gabrieldemarmiesse gabrieldemarmiesse left a comment

Choose a reason for hiding this comment

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

The implementation looks good, it would be kind of you to implement the new tests. Thanks a lot!

.format(type(grouped_elements), grouped_elements))
else:
raise TypeError(
"Expected list of elements to be documented, is of type {}: {}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Expected list of elements to be documented, is of type {}: {}"
"Expected list or dict of elements to be documented, is of type {}: {}"

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants