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

Html generation leaves out the first character from python imports #84

Open
IAlwaysBeCoding opened this issue Jan 14, 2016 · 11 comments

Comments

Projects
None yet
2 participants
@IAlwaysBeCoding
Copy link
Contributor

commented Jan 14, 2016

So, I found an odd bug. Don't really know how to fix it , but I know how to get around it.

Here is a screenshot:
https://imgur.com/w6Z5Ro3

Example:

from collections import OrderedDict

Will produce source code without the first character, only if the first line is blank. To fix this, just make sure the first line isn't blank, and will produce the correct output.

Also, just wanted to say that your project is FREAKING amazing. I am thinking of using it for stitching together something simple that I can use to upload my documentation to gitbook.com . Sphinx is kinda too big, although is great it doesn't work well with Markdown. Plus, I saw that your project also processes any markdown too in doc strings which is super useful. Thanks for this god-send project, just what I was looking for .

@BurntSushi BurntSushi added the bug label Jan 14, 2016

@BurntSushi

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2016

That is a strange bug indeed! I haven't reproduced it yet, but I wonder, is the HTML correct and this is just a styling issue? Or is the first character actually dropped from the output altogether? (Either would be weird!)

@IAlwaysBeCoding

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2016

I am not sure, yet haven't taken a look at the code. Here is the html of that screenshot
https://gist.github.com/IAlwaysBeCoding/3192fe35a827646a2807.

And it is actually stripping the first character of each line, the html shows that it is being stripped and not just a styling issue.

Now , you made me curious let me take a look why that is happening and do a PR.

@IAlwaysBeCoding

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2016

Okay, found the bug. It's here https://github.com/BurntSushi/pdoc/blob/master/pdoc/templates/html.mako#L41

It basically tries to get the indent of the first line, but since the first line is an empty line the indent is 1 and not the expected 0. So, on line 41 basically says give me every single line from source code but starting from the index whose value equals indent .

I am not sure why it needs to get the indent though, so am afraid of removing that thinking it might mess something else. However, if you remove line 41 then it should work.

@BurntSushi

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2016

@IAlwaysBeCoding the reason why it's mucking with indents is if the code is indented, then it doesn't look so good to just put that in <pre> tags as-is (since the indent will show). In the case of showing the full module, mucking with the indent isn't needed, which is probably why it seems confusing in this context. But, for example, showing the source code of methods on classes, the dedenting code is pretty nice for a clean display.

I think I'd probably recommend fixing the indent code. That is, compute the indent based on the first non-empty line instead of the first line. (There may be other cases where this fails, but I'd be happy with that simple fix for now!)

@IAlwaysBeCoding

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2016

@BurntSushi Oh okay! Then in that case, we can do a really quick fix. Just check if the first line just contains white space, if so then set the base indent to 0 if not then set it to whatever the base indent is. Let me do a quick pull request. That should fix that for now for that edge case and leave the indended functionality behind like it was.

@BurntSushi

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2016

How about:

base_indent = 0
for line in line:
    if len(line.strip()) > 0:
        base_indent = len(indent.match(lines[0]).group(0))
        break
@IAlwaysBeCoding

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2016

I had this in mind,literally I was 30 seconds away from finishing the pull request. What do you think
That line goes after line 40

base_indent = 0 if lines[0].isspace() else base_indent
@BurntSushi

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2016

@IAlwaysBeCoding Well, what happens if there are two preceding empty lines? :P Might as well just find the first non-empty line and base the indent off of that. (I'm not sure if you're worried about performance or not, but if you are, you shouldn't be. :-))

@IAlwaysBeCoding

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2016

@BurntSushi I am confused though, like for example on my sublime editor. I can't really run any python if the first line has any spaces as indentation starting out because I get the IndentationError: unexpected indent error. Maybe, I just haven't really seen a scenario like that yet. Anyways, both of our solutions fixes the problem, but yours does make it better for people like us that start their code like 1-2 lines after.

@BurntSushi

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2016

@IAlwaysBeCoding I think I'm just thinking about other places, like methods, but I think their code blocks are probably trimmed anyway. Nevertheless, I think I prefer the more robust way of doing it (find first non-empty line) for now. Please feel free to submit a PR with it. :-) Thanks!

@IAlwaysBeCoding

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2016

Okay, I submitted a pull request. If there is anything else you would like changed let me know. Anyways, I think am going to be playing a lot more with pdoc from now on. Great project though!

IAlwaysBeCoding added a commit to IAlwaysBeCoding/pdoc that referenced this issue Jan 14, 2016

IAlwaysBeCoding added a commit to IAlwaysBeCoding/pdoc that referenced this issue Jan 14, 2016

Fix html.mako bug mitmproxy#84
This should fix it, @BurntSushi . I fixed a typo where you put `for line in line` instead of `for line in lines` . Otherwise , everything works as I finished testing it to make sure the source code was displayed right.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.