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

Python 3 compatibility fixes #65

Open
wants to merge 35 commits into
base: master
Choose a base branch
from
Open

Python 3 compatibility fixes #65

wants to merge 35 commits into from

Conversation

lelit
Copy link

@lelit lelit commented Feb 9, 2014

The following changes were needed to successfully use slimit with Python 3 in my own project.

I had to use current (not yet released) ply though, with a tiny fix of mine for its issue 44.

lelit and others added 30 commits February 9, 2014 08:10
Use plain unicode strings instead, doubling backslashes when needed.
These values are expected by ply to be module names, not module
instances. ply 3.6 will crash if actual modules are passed.
characters (e.g. someObj[':']) would incorrectly
have the brackets/quotes removed.  See slimit issue rspivak#47:
rspivak#47 (was easier to fix than
I thought 👍).
Use plain unicode strings instead, doubling backslashes when needed.
metatoaster added a commit to calmjs/calmjs.parse that referenced this pull request Jun 8, 2017
- Also note that the following issues were addressed, where applicable
  to the lexer or parser.

  - rspivak/slimit#52
  - rspivak/slimit#54
  - rspivak/slimit#57
  - rspivak/slimit#59
  - rspivak/slimit#62
  - rspivak/slimit#65
  - rspivak/slimit#70
  - rspivak/slimit#73
  - rspivak/slimit#79
  - rspivak/slimit#81
  - rspivak/slimit#82
  - rspivak/slimit#90

- Will get the release out when I get some sleep.
metatoaster added a commit to metatoaster/calmjs.parse that referenced this pull request Jun 13, 2017
- Changes via rspivak/slimit@8f9a39c776 (which got pulled in via import
  of rspivak/slimit#65 did not have tests, so it naturally never worked.
@gforcada
Copy link

gforcada commented Oct 2, 2018

Meanwhile #102 was merged already. Should this be closed? 🤔

@lelit
Copy link
Author

lelit commented Oct 2, 2018

Probably yes. I switched to https://github.com/calmjs/calmjs.parse months ago, so I can't say if #102 implements everything proposed here, though.

@metatoaster
Copy link

They should all be more or less implemented since I cut it from the @lelit fork, however I did make some significant changes as some parts of the fork, such as at places where some new changes were done inefficiently. The other major thing is that that there are a pile of bug fixes that simply couldn't easily be done without correcting some fundamental flaws, such as parsing and the implementation of source map generation. The changelog file has references to all of the issues reported on slimit and the calmjs.parse issue tracker.

Since @gforcada is now here in this thread, if Plone 5 is still going to maintain the existing native JavaScript minification function, the minify print function just had a slight import/function name change if the latest calmjs.parse release is to be used; a brief example is in the documentation.

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.

7 participants