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

Fix missing import in doc macro expansion #1679

Merged
merged 1 commit into from
Sep 24, 2018

Conversation

brandonwillard
Copy link
Member

The doc macro expands without importing importlib.import_module, so, if the expansion namespace doesn't have import_module present, doc will fail. This PR corrects that and introduces a missing test for the doc macro.

Copy link
Member

@Kodiologist Kodiologist left a comment

Choose a reason for hiding this comment

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

Don't forget to update NEWS.

@Kodiologist
Copy link
Member

I didn't realize we had the empty? function. It seems a little pointless, since not should do the same thing in most cases where you'd use empty?.

@brandonwillard
Copy link
Member Author

I think this bug was introduced by the use of importlib.import_module (via #1672), so should it be in the same NEWS that reports the addition of #1672?

Also, I was thinking of putting in a PR that rewrites the doc macro so that it simply works with everything (i.e. tags, macros, functions). It's much less confusing to get documentation in Hy that way, so, if anyone else is interested, we could entirely obviate this PR with that one.

@Kodiologist
Copy link
Member

Kodiologist commented Sep 9, 2018

I think this bug was introduced by the use of importlib.import_module (via #1672)

Oh, okay, there's no need for a NEWS item if this bug was a new regression.

Also, I was thinking of putting in a PR that rewrites the doc macro so that it simply works with everything (i.e. tags, macros, functions). It's much less confusing to get documentation in Hy that way, so, if anyone else is interested, we could entirely obviate this PR with that one.

That sounds fine to me.

@gilch
Copy link
Member

gilch commented Sep 10, 2018

rewrites the doc macro so that it simply works with everything

I'm less convinced. How should (doc foo) behave if foo names both a global and a macro? I implemented doc #1415 (and #doc) more as a stopgap until #1416, which would let us just use help for everything.

@brandonwillard
Copy link
Member Author

It should print the documentation of both or allow one to specify/choose.

@gilch
Copy link
Member

gilch commented Sep 10, 2018

@brandonwillard see the discussion in #1415 for why that's a bad idea.

@brandonwillard
Copy link
Member Author

I was looking over #1416 yesterday; it's next on my list. I've been doing all this recent work in preparation for larger macro-oriented changes, so I'll need your input on that later, as well as #1407 and #277.

Using help would be nice, too, if only for leveraging its already established role. On a side note, I think the focus of this topic should probably be on broader pydoc functionality/inter-op. That said, the doc we're talking about is probably more related to pydoc.doc/pydoc.render_doc than help.

Otherwise, the discussion in #1415 does not make a clear case for why a type-narrowing option — or something similar — is objectively "bad". It looks like a matter of preference, especially since the current implementation only separates the cases based on a syntax mnemonic (instead of, for instance, a naming scheme like doc-macro, doc-tag and possibly a doc-defn).

I don't really have a preference either way, but I do think there's value in having the ability to obtain all the information in one place.

@gilch
Copy link
Member

gilch commented Sep 10, 2018

does not make a clear case for why a type-narrowing option — or something similar — is objectively "bad".

This is moot if we can use help for everything. doc was only ever meant as a stopgap.

But it is a serious usability concern, not just a personal preference. The function you use when you've forgotten how to use things should be easy to remember how to use.

If you have to switch to your browser and google how to do it, what's the point of having docs in your repl at all? Just google the macro or whatever. If it's not simple, we may as well not have it. That means a single-argument form with a simple name. It's already bad that I made a doc separate from help. As it is now, all you have to remember is doc because (doc doc) will tell you how to use the rest if you forget.

There may be other approaches besides the one I chose that would meet this requirement ("something similar"? I can think of a few), but making things complicated is not it. Printing out all three is less bad, but still problematic when the docs are long.

the focus of this topic should probably be on broader pydoc functionality/inter-op.

Sure, feel free to open new issues on that point. Also see #1019, and #1044, which are sort of related.

@Kodiologist
Copy link
Member

I don't think I understand what you want. If there's a function, macro, and tag macro with the same name, and you don't want doc (or help) to print out all the options, and you don't want to let the user select which to print out, then what is it supposed to do?

@gilch
Copy link
Member

gilch commented Sep 11, 2018

Fixing doc per this PR is fine for the time being.

If we put macros in a __macros__ global or something like that #1416, then we shouldn't need the doc macro at all. You'd use help(foo) in the debugger for a runtime foo and probably help(__macros__.foo) for help with the macro foo. (Or (help foo) at the Hy repl or if we get a Hy debugger, etc.) This is like specifying which namespace, but the __macros__ object would be discoverable using dir(), so it's easy to find if you forget. This would be a lot more consistent with how help works in Python currently.

Tag macros might be in a similar __tags__ or something. We do want separate namespaces for macros and global functions for operator shadowing to work, but I'm not sure if we want a third namespace for tag macros or not. If we reworked them to be more like Clojure's tagged literals, instead of sugar for one-argument macros, that might change the answer.

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.

3 participants