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

remove import of unicode_literals #1887

Merged
merged 85 commits into from
Mar 3, 2016
Merged

remove import of unicode_literals #1887

merged 85 commits into from
Mar 3, 2016

Conversation

pkess
Copy link
Contributor

@pkess pkess commented Feb 17, 2016

Hi,
as mentioned in #1484 i started a new branch to remove the import of unicode_literals.
With the remove of this import we should explicit mark some strings in the code as unicode strings. In my opinion all output strings and strings compared against some input should be marked as unicode strings.
Please leave your comments!

@pkess pkess changed the title Started remove import of unicode_literals remove import of unicode_literals Feb 17, 2016
@untitaker
Copy link
Contributor

You need to be careful with converting too eagerly to unicode strings: Python 2's support for equality comparisons is garbage and doesn't work if you're dealing with non-ascii data.

Really you need to deal with three types: u"", b"", "".

Ideally we'd do all of the following in one huge step:

  • Port to click
  • Remove unicode literals imports
  • Drop Python 2

@sampsyo
Copy link
Member

sampsyo commented Feb 17, 2016

Awesome! Thanks for taking the first step.

Yes, the majority of string literals should be u literals. A few rules of thumb come to mind:

  • log.debug/warn/info/etc('foo' ...: Always u.
  • Pieces of filenames: Always b, unless it's in some parts of the destination method before the path is encoded.
  • URLs and data for HTTP requests: Probably unannotated ("native") string literals.
  • Arguments to exceptions: Almost always u.
  • print_ and such: Always u.

We should also consider running unicode-nazi occasionally to catch some bad comparisons.

@pkess
Copy link
Contributor Author

pkess commented Feb 19, 2016

What will we do with SQL statements?

@sampsyo
Copy link
Member

sampsyo commented Feb 20, 2016

Good question. After a little googling, I think the answer is native/unannotated strings.

@pkess
Copy link
Contributor Author

pkess commented Feb 28, 2016

Ok,
there is only one file left with import of unicode_literals: test/test_iu
I started to remove it in https://github.com/pkess/beets/tree/no_unicode_literals_test_ui
There are two errors i can't explain or debug. Could anyone help here?

@pkess
Copy link
Contributor Author

pkess commented Feb 28, 2016

Please review the above commits and feel free to leave comments

pkess and others added 2 commits February 28, 2016 13:47
* nosetests ´test.test_ui´ fails for two tests
@untitaker
Copy link
Contributor

@pkess I opened a PR. I still get one failure, but I haven't looked into it yet.

@pkess
Copy link
Contributor Author

pkess commented Feb 28, 2016

Hi, thank you for your support. I don't get any failures by running tox on my local machine. I will merge the here to see if travis reports any failures

Most commonly, this sticks with:

    log.debug(
        'some long message here'
    )

instead of placing the closing ) at the end of the string literal.
Since the list is short enough now, we don't need parentheses for the line
wrap. This is a little less ugly.
@sampsyo
Copy link
Member

sampsyo commented Feb 28, 2016

AWESOME! ✨ This was a truly huge amount of work. You even got the coveted "diff too large to display" message on GitHub. 🐟

I've scanned over the entire diff and changed a few tiny style things. I also synced with the master branch so this PR can merge cleanly. Everything looks great. Unless there's anything else outstanding, I move to merge this now.

sampsyo added a commit that referenced this pull request Feb 28, 2016
This adds the [flake8-future-import][f] plugin for flake8, which enforces the
standard set of `__future__` imports at the top of all Python files. This
revealed a fair number of files that need to be fixed.

To be revisited after #1887 is merged.

[f]: https://github.com/xZise/flake8-future-import
@pkess
Copy link
Contributor Author

pkess commented Mar 2, 2016

Ok, would you like to hit the merge button?

@pkess
Copy link
Contributor Author

pkess commented Mar 2, 2016

I just added a wiki page with the conventions for strings discussed here:
https://github.com/beetbox/beets/wiki/String-literals
I hope you agree with this

@sampsyo
Copy link
Member

sampsyo commented Mar 3, 2016

Woohoo! ✨

And yes, that wiki page looks great.

sampsyo added a commit that referenced this pull request Mar 3, 2016
remove import of unicode_literals
@sampsyo sampsyo merged commit abf2511 into master Mar 3, 2016
wisp3rwind pushed a commit to wisp3rwind/beets that referenced this pull request Apr 16, 2016
This adds the [flake8-future-import][f] plugin for flake8, which enforces the
standard set of `__future__` imports at the top of all Python files. This
revealed a fair number of files that need to be fixed.

To be revisited after beetbox#1887 is merged.

[f]: https://github.com/xZise/flake8-future-import
@arcresu arcresu deleted the no_unicode_literals branch April 24, 2019 05:02
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.

3 participants