axiros / terminal_markdown_viewer

Styled Terminal Markdown Viewer
Other
1.81k stars 106 forks source link

Improve Python 3 compatibility, resolve PEP8 errors, and fix an undefined variable #28

Closed smkent closed 7 years ago

looneychikun commented 7 years ago

I love the pep8 fixes.

This looks to be complaint with python3, can you confirm that? Have a project i want to use this for arbitrary cli face documentation, but its all py3. Want to unify our docs with readmes based in the faces of the cli.

axgkl commented 7 years ago

hi, sorry for the silence. I took most of your stuff and merged manually into a new version (had a few other improvements), so I'm closing this PR, hope you're still ok with it allthough I took not everything.

Unfortunatelly Py3 is not really usable yet at this point by maybe its a good state now to fix the remaining probs:

screen shot 2016-11-03 at 20 38 00

notes:

the rest plus the bugs and omittable declarations: all well.

=> many thanks for the contrib.

axgkl commented 7 years ago

One thing, curious: I wonder which tool suggests to wrap all iterables into list() (?) Is this to avoid silent mutations or whats the reason for this?

smkent commented 7 years ago

Thanks for the update!

One thing, curious: I wonder which tool suggests to wrap all iterables into list() (?) Is this to avoid silent mutations or whats the reason for this?

I'm using flake8 with Syntastic to identify PEP8 errors and such. flake8 doesn't complain about iterables themselves, buy Python 3 throws an exception when doing indexing on a dictionary's .keys() result (apparently it's no longer indexable in Python 3):

$ python3 mdv/markdownviewer.py                                                 
Using sample markdown:
Traceback (most recent call last):
  File "mdv/markdownviewer.py", line 1255, in <module>
    run()
  File "mdv/markdownviewer.py", line 1251, in run
    print(run_args(args))
  File "mdv/markdownviewer.py", line 1217, in run_args
    ,tab_length    = args.get('-b', 4)
  File "mdv/markdownviewer.py", line 906, in main
    make_sample()
  File "mdv/markdownviewer.py", line 259, in make_sample
    for ad in admons.keys()[:1]:
TypeError: 'dict_keys' object is not subscriptable

Wrapping the dictionary .keys() calls with list() prevents that (at the cost of creating a new object). There may be a better solution; my goal with this pull request was to just get mdv working at a basic level with Python 3.

axgkl commented 7 years ago

reg. list:

ah ok, its about the loops over dict keys. Yeah, they changed dict implementation a lot and I must admit that I do like what they did in that area, all dicts are sorted now and pretty space effective (thats what the company guru told me recently ;-) ).

reg. py3 in general and some notes regarding you = have looked into the code, which is really hard to read, since a first iteration with me = learning markdown down the way . Take it as an excuse ;-)

Py3 was and still is not really usable for my company because of what we call the unicode disaster, we do lots of I/O . But there is reason to believe that the problem goes away when they finally go defaultencoding utf-8, like, pretty much, the rest of the world and even windows. Then we'll turn to py3 as well and I guess over wintertime I find the time to also closer look into a better, compatible version of mdv. The current version was really a hack but thanks anyway to have helped improving it.

So if you can wait like a few months with py3 then I'm pretty positive that you'll get it within a few months. If you need it now than I guess you might "want" to have another look into it again, for the remaining probems with even simple markdown.

looneychikun commented 7 years ago

I'll take another pass at this with py3. Trying to avoid documentation in 2+ places, with nice consistent formatting.

This really isnt the thread for this, but i imagine this blows up on windows. We have nix, mac, and windows users using the cli. My gut tells me this implementation would at worst destroy the pathetic windows console, and at best just have control characters everywhere. If I can find the time I can look into os detection for windows console.