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

Fixed broken URL in help output #1051

Merged
merged 5 commits into from Jan 16, 2017

Conversation

Projects
None yet
2 participants
@jlstevens
Member

jlstevens commented Jan 11, 2017

This PR addresses #978. The code change is trivial but it is likely that the test data will need updating.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 11, 2017

I'm a little surprised that the tests passed...maybe this output is being sent to the pager and therefore isn't captured?

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 16, 2017

Yes, I think that's it. One issue I have is that custom Elements will also point to non-existent entries in the elements tutorial. Got any ideas to disable that?

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 16, 2017

I agree that is an issue though not a particularly major one. My only suggestion is to suggest the help URL only if the element is one of the ones we offer. Do we have such a list?

Maybe the top level __init__.py could set a global variable (list of all our elements) in core.pprint. At least this would avoid having to pass this list though help, then Store, then into the pretty printer and all its methods...

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 16, 2017

Could probably just iterate over the hv.element namespace and find all Element types to get that list.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 16, 2017

I suppose so given that seaborn elements aren't in that tutorial anyway. Should I really do the same for Containers or can I just ignore the possibility of custom Containers? I doubt people will want to build new containers often...

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 16, 2017

Should I really do the same for Containers or can I just ignore the possibility of custom Containers?

Fine to ignore right now I think.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 16, 2017

Added filtering of elements as suggested:

image

Versus a core element:

image

Note this PR isn't quite ready to merge. I might as well fix the issue about showing help for the active backend while I am working on this code.

@@ -25,6 +25,7 @@
from .operation import ElementOperation, MapOperation, TreeOperation # noqa (API import)
from .element import * # noqa (API import)
from . import util # noqa (API import)
from .element import __all__ as elements_list

This comment has been minimized.

@philippjfr

philippjfr Jan 16, 2017

Member

That doesn't seem right, it should at least filter for objects of type Element.

This comment has been minimized.

@philippjfr

philippjfr Jan 16, 2017

Member

I know hv.element.__all__ filters for ViewableElement but I think that includes some other types as well, e.g. Overlays. So maybe just tighten that check to use Element instead of ViewableElement.

This comment has been minimized.

@jlstevens

jlstevens Jan 16, 2017

Member

The problem is that __all__ contains strings (which I want) but doing tighter subclass checks requires a list of those classes. I could iterate over locals but I find that pretty ugly. What would you suggest?

This comment has been minimized.

@philippjfr

philippjfr Jan 16, 2017

Member

I'd change line 99 of hv.element.__init__ to:

return issubclass(obj, Element)

This comment has been minimized.

@jlstevens

jlstevens Jan 16, 2017

Member

Done! I was hesitant in case anything was expecting to find ViewableElements...

This comment has been minimized.

@philippjfr

philippjfr Jan 16, 2017

Member

Appears you were right being hesitant, but it really shouldn't be relying on importing Overlay via hv.element.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 16, 2017

Ok. If the tests pass I think this PR is ready for final review/merge.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 16, 2017

Looks good, merging.

@philippjfr philippjfr merged commit a026702 into master Jan 16, 2017

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.06%) to 76.915%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the help_url_fix branch Jan 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment