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 cgi escape error python 3.8+ #37

Merged
merged 1 commit into from
May 23, 2021

Conversation

garaud
Copy link
Contributor

@garaud garaud commented Apr 23, 2021

related to #36

@codecov
Copy link

codecov bot commented Apr 23, 2021

Codecov Report

Merging #37 (70f231e) into dev (ce68b08) will not change coverage.
The diff coverage is 0.00%.

❗ Current head 70f231e differs from pull request most recent head 89a9297. Consider uploading reports for the commit 89a9297 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##              dev      #37   +/-   ##
=======================================
  Coverage   51.60%   51.60%           
=======================================
  Files           6        6           
  Lines         500      500           
=======================================
  Hits          258      258           
  Misses        242      242           
Impacted Files Coverage Δ
justext/__main__.py 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 508b4c3...89a9297. Read the comment docs.

Copy link
Owner

@miso-belica miso-belica left a comment

Choose a reason for hiding this comment

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

I guess this is Karma :D

Thank you for the fix but I think it's not OK to drop support for Python 2 for such a minor thing. I know Python 2 is dead and I like it that way but I think the compatibility can be achieved by trying to import escape from cgi and if it fails then html. Can you please change the PR so that it's compatible with both and Python 2 support is preserved please?

try:
    from cgi import escape
except ImportError:
    from HTML import escape

Thank you.

justext/__main__.py Outdated Show resolved Hide resolved
@garaud
Copy link
Contributor Author

garaud commented Apr 26, 2021

About the Python 2 support, I'm quite surprised but it's fine for me :)

I drop the latest commit of my PR which modified the README and the setup.py about Python 2.

I can update my PR with a try...except in the imports. I'm quite more annoyed with the line fp_out = stream_writer(sys.stdout.buffer) which works in py3. In Python 2, this was fp_out = stream_writer(sys.stdout) but raised an exception in py3. I'm working on it.

@garaud
Copy link
Contributor Author

garaud commented Apr 26, 2021

@miso-belica I updated my PR.

Copy link
Owner

@miso-belica miso-belica left a comment

Choose a reason for hiding this comment

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

I believe that after a few more small tweaks it will be perfect :)

justext/__main__.py Outdated Show resolved Hide resolved
The 'cgi.escape' function was deprecated since Python 3.2. It has been
removed from Python 3.8.

Use 'html.escape' instead.

fix miso-belica#36
Copy link
Owner

@miso-belica miso-belica left a comment

Choose a reason for hiding this comment

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

Thank you :)

@miso-belica miso-belica merged commit 70d4c5a into miso-belica:dev May 23, 2021
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.

None yet

2 participants