Skip to content
This repository has been archived by the owner on Oct 21, 2022. It is now read-only.

Python3 unicode - Fixing issue #24 -NameError: name 'unicode' is not defined #47

Merged
merged 3 commits into from
Apr 11, 2017

Conversation

RockyRoad29
Copy link
Contributor

Hi,

I read the few suggestions and fix proposals for bug #24, but here's my own approach,
feel free to comment.

So the problem is in ltmain.py::ensureUtf() which is used when calling
compile().

/source/ can either be a Unicode string, a Latin-1 encoded string or
an AST object.

So if compile() expects unicode (let's ignore old-time latin),
it is bytes.decode() that we'd need... and the source encoding might
depend on the user's platform ?

Here's my proposal :

    def ensureUtf(s, encoding='utf8'):
      """Converts input to unicode if necessary.

      If `s` is bytes, it will be decoded using the `encoding` parameters.

      This function is used for preprocessing /source/ and /filename/ arguments
      to the builtin function `compile`.
      """
      # In Python2, str == bytes.
      # In Python3, bytes remains unchanged, but str means unicode
      # while unicode is not defined anymore
      if type(s) == bytes:
        return s.decode(encoding, 'ignore')
      else:
        return s

I tested it with python2.7.11 and python3.5.1 on my archlinux platform.

Then, if accepted it makes sense to rename the function to reflect its actual role,
which I did in a later commit.

... edit: Ooops, sorry, the rename was not complete, please check the last commit(s) from my branch.

@kenny-evitt
Copy link

@RockyRoad29 Thanks for this. I wish I was able to provide useful feedback. Your changes look reasonable to me anyways! Hopefully someone else will step in and review your changes.

We (the open source team) are looking for someone to take ownership of this plugin. Let me know (here or in the Gitter room for our main repo) if you're interested. Without an active maintainer, and not knowing enough Python to be able to judge, there's no one to merge this (or any other) pull request.

@RockyRoad29
Copy link
Contributor Author

Hi the LightTable team,

Thank you very much for your comment Kenny.

Sorry for being to long to reply, I've not been much on the computer
lately and missed some emails amongst the mass I get every day.

It's a pity that nobody else tried to review the changes since.
I'd love to be able to help, but I'm not sure to find
enough time for it, some coworkers would be most useful.

I'm pretty confident that my patch will fix things maybe beyond this
python3 compatibility issue, but we need testers !

Regards,
"Rpcky Rpad"

On 19/01/16 23:23, kenny-evitt wrote:

@RockyRoad29 Thanks for this. I wish I was able to provide useful feedback. Your changes look reasonable to me anyways! Hopefully someone else will step in and review your changes.

We (the open source team) are looking for someone to take ownership of this plugin. Let me know (here or in the Gitter room for our main repo) if you're interested. Without an active maintainer, and not knowing enough Python to be able to judge, there's no one to merge this (or any other) pull request.


Reply to this email directly or view it on GitHub:
#47 (comment)

@kenny-evitt
Copy link

@RockyRoad29 You can always run your own version of the Python plugin and you could also create your own plugin, that others could install in LT too, by forking this repo. That's probably a lot less work than committing to taking over this plugin.

@RockyRoad29
Copy link
Contributor Author

@kenny-evitt This PR already links to my fork ... here it is, available for all:
RockyRoad29/Python :-)

@kenny-evitt
Copy link

@RockyRoad29 Thanks! Any other open PRs on this repo that you merged into your fork?

@stemd
Copy link
Collaborator

stemd commented Jun 7, 2016

I applied this in my install and it seems fine (I use pylint with other editors and enforce PEP8 in my code).

@kenny-evitt
Copy link

@stemd It's your call now! If you think it's good, merge away.

By-the-way, ping me, in an issue or in Gitter, when you're ready to release a new version of the plugin (so everyone can upgrade in LT – tho the in-LT upgrade is still broken! [LightTable/LightTable#2138]).

@hellerbarde
Copy link

Has this been merged? If not, what's stopping it?

@sbauer322 sbauer322 merged commit 8ac9fa4 into LightTable:master Apr 11, 2017
@sbauer322
Copy link
Contributor

It has not been merged yet as this plugin is in need of a maintainer, @hellerbarde.

Based on feedback here and comments on issue #54 I am comfortable with merging this PR and will do so. However, I am not particularly familiar with Python or use it on a daily basis so there is a potential cause for concern with cutting a release (and merging other Python plugin PRs).

Until someone comes along to maintain the plugin (and make a release), you can pull down the repo and use it instead of the last proper plugin release.

@Dangthrimble
Copy link

I have just tried this fix with Python 3.3.5 on my Win10 64-bit PC and it fixes the problem for me,

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants