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

NEW: Add type, format, transformer info to plugin pages #435

Conversation

Oddant1
Copy link
Member

@Oddant1 Oddant1 commented Oct 9, 2019

Adds dummy lists of types, formats, and transformers to plugin pages. These list entries currently have no link associated with them. This code is also currently producing this on the "Available plugins" page for plugins with types, formats, or transformers:


Selection_104


There are no list entries there because the pages the entries would be linking to do not yet exist.


Co-Authored-By: ChrisKeefe <[email protected]>

@thermokarst
Copy link
Contributor

Hey @Oddant1, have you chatted with @ChrisKeefe about reviewing this, first? If he has the availability to take a first pass before me, well, that would be great. Thanks!

@Oddant1
Copy link
Member Author

Oddant1 commented Oct 9, 2019

@thermokarst I will definitely do that. Also should add him as a co-author

@Oddant1 Oddant1 requested review from ChrisKeefe and removed request for thermokarst October 9, 2019 20:06
@Oddant1 Oddant1 changed the title NEW: Add type, format, transformer info to plugin pages NEW: Add type, format, transformer info to plugin pages Co-Authored-By: ChrisKeefe <[email protected]> Oct 9, 2019
@Oddant1 Oddant1 changed the title NEW: Add type, format, transformer info to plugin pages Co-Authored-By: ChrisKeefe <[email protected]> NEW: Add type, format, transformer info to plugin pages Oct 9, 2019
@Oddant1
Copy link
Member Author

Oddant1 commented Oct 9, 2019

Unfortunately GitHub doesn't seem to allow editing of commits to add co-authors from inside GitHub, but I think that last commit should add Chris

@Oddant1
Copy link
Member Author

Oddant1 commented Oct 9, 2019

Sigh nope, Hang on

@thermokarst
Copy link
Contributor

Just put the Co-Authored-By tag in the main PR description - I will drop that into the commit message when it gets squashed and merged.

@Oddant1
Copy link
Member Author

Oddant1 commented Oct 9, 2019

@thermokarst, sounds good, although it also looks like I have the wrong email for Chris? Because it added him and then the commit StackOverflow said I needed to make to not blow everything up took him off 😆, but when it added him it didn't create a link to his account

@Oddant1 Oddant1 changed the title NEW: Add type, format, transformer info to plugin pages NEW: Add type, format, transformer info to plugin pages Co-Authored-By: ChrisKeefe <[email protected]> Oct 9, 2019
@thermokarst thermokarst changed the title NEW: Add type, format, transformer info to plugin pages Co-Authored-By: ChrisKeefe <[email protected]> NEW: Add type, format, transformer info to plugin pages Oct 9, 2019
@@ -49,12 +49,18 @@ def generate_rst(app):
transformers_list = []

for from_type, to_type in plugin.transformers:
from_type = repr(from_type).split('.')[-1].replace("'>", '')
to_type = repr(to_type).split('.')[-1].replace("'>", '')
from_type = repr(from_type) \
Copy link
Contributor

Choose a reason for hiding this comment

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

DBC:

Please see this PEP for a simpler way to do this: https://www.python.org/dev/peps/pep-3155/

An example in QIIME 2 can be found here: https://github.com/qiime2/qiime2/blob/46541f547c92714079dbd0342e10a85b74522305/qiime2/core/util.py#L28

Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought, you might just want to look at using get_view_name, outright.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you use get_view_name outright, we will probably need to open a tiny PR on the framework to expose that util through sdk (right now it is only in core).

@Oddant1
Copy link
Member Author

Oddant1 commented Nov 4, 2019

@7d0436e Moves the types, formats, and transformers to their own pages, these pages are not finished at this time. Additionally, the docs do not build without warnings right now, it warns that some of the .rst files that are being used as templates are not in toctrees anywhere, and I don't know how to fix it yet.

@Oddant1 Oddant1 assigned thermokarst and Oddant1 and unassigned thermokarst Nov 4, 2019
@Oddant1
Copy link
Member Author

Oddant1 commented Nov 5, 2019

I've noticed some oddities in the formats sets we've created, only two items in the Exportable Formats set actually say they're formats. One of them appears to be a pipeline which doesn't seem like it should be there (see the last entry in the list).


Selection_002

All these entries in the set came from the dictionary of transformers that already existed on the PluginManager object. We just indexed into it using the canonical formats. Seen below is the entry in the transformers dictionary corresponding to that sklearn.pipeline.Pipeline entry in the exportable formats set.


Selection_003

I'm not sure if this is actually a problem, but it doesn't seem right to me.

@Oddant1 Oddant1 assigned thermokarst and unassigned Oddant1 Nov 5, 2019
format-list.rst Outdated Show resolved Hide resolved
formats-list.rst Outdated Show resolved Hide resolved
source/formats-list.rst Outdated Show resolved Hide resolved
source/transformers.rst Outdated Show resolved Hide resolved
source/types-list.rst Outdated Show resolved Hide resolved
types_list.rst Outdated Show resolved Hide resolved
@thermokarst
Copy link
Contributor

Yikes! Not sure how those all sent as individual messages, meant to submit as a comment-review. Sorry!

@Oddant1
Copy link
Member Author

Oddant1 commented Nov 6, 2019

b9a5f90 Still gives warnings on the templates not being included in any toctree

@thermokarst thermokarst assigned ebolyen and unassigned thermokarst Jun 24, 2022
@lizgehret
Copy link
Member

This is a nice idea, I wonder if we can include something like this after the November release when we re-vamp the dev docs. @ebolyen any thoughts?

@ebolyen
Copy link
Member

ebolyen commented Sep 21, 2022

Closing in favor of #543

@ebolyen ebolyen closed this Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants