-
Notifications
You must be signed in to change notification settings - Fork 208
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
python3 compatibility #187
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
4b2e1c4
Use unicode_literals.
e702091
Suppress false 2to3 positive.
0f99658
Use compat functions to convert bytes/unicode.
bfd43ad
Enable python3 tests.
d5321db
Use deterministic UIDs on event from string tests.
f347562
Use click's styling functions rather than our own.
2163464
Fix bytestrings being badly rendered on the UI.
d65e787
Avoid casting output to bytestring.
7df2f83
Add an XXX task to later fix a string generation.
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,9 @@ | |
# WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. | ||
# | ||
|
||
from click import echo | ||
from __future__ import unicode_literals | ||
|
||
from click import echo, style | ||
|
||
import datetime | ||
import itertools | ||
|
@@ -30,12 +32,13 @@ | |
import textwrap | ||
|
||
from khal import aux, calendar_display | ||
from khal.compat import to_unicode | ||
from khal.khalendar.exceptions import ReadOnlyCalendarError | ||
from khal.exceptions import FatalError | ||
from khal.khalendar.event import Event | ||
from khal import __version__, __productname__ | ||
from khal.log import logger | ||
from .terminal import bstring, colored, get_terminal_size, merge_columns | ||
from .terminal import colored, get_terminal_size, merge_columns | ||
|
||
|
||
def construct_daynames(daylist, longdateformat): | ||
|
@@ -50,9 +53,9 @@ def construct_daynames(daylist, longdateformat): | |
""" | ||
for date in daylist: | ||
if date == datetime.date.today(): | ||
yield (date, u'Today:') | ||
yield (date, 'Today:') | ||
elif date == datetime.date.today() + datetime.timedelta(days=1): | ||
yield (date, u'Tomorrow:') | ||
yield (date, 'Tomorrow:') | ||
else: | ||
yield (date, date.strftime(longdateformat)) | ||
|
||
|
@@ -109,14 +112,14 @@ def get_agenda(collection, locale, dates=None, | |
if len(events) == 0 and len(all_day_events) == 0 and not show_all_days: | ||
continue | ||
|
||
event_column.append(bstring(dayname)) | ||
event_column.append(style(dayname, bold=True)) | ||
events.sort(key=lambda e: e.start) | ||
for event in itertools.chain(all_day_events, events): | ||
desc = textwrap.wrap(event.compact(day), width) | ||
event_column.extend([colored(d, event.color) for d in desc]) | ||
|
||
if event_column == []: | ||
event_column = [bstring('No events')] | ||
event_column = [style('No events', bold=True)] | ||
return event_column | ||
|
||
|
||
|
@@ -134,6 +137,8 @@ def __init__(self, collection, date=[], firstweekday=0, encoding='utf-8', | |
firstweekday=firstweekday, weeknumber=weeknumber) | ||
|
||
rows = merge_columns(calendar_column, event_column) | ||
# XXX: Generate this as a unicode in the first place, rather than | ||
# casting it. | ||
echo('\n'.join(rows).encode(encoding)) | ||
|
||
|
||
|
@@ -144,7 +149,9 @@ def __init__(self, collection, date=None, firstweekday=0, encoding='utf-8', | |
term_width, _ = get_terminal_size() | ||
event_column = get_agenda(collection, dates=date, width=term_width, | ||
show_all_days=show_all_days, **kwargs) | ||
echo('\n'.join(event_column).encode(encoding)) | ||
# XXX: Generate this as a unicode in the first place, rather than | ||
# casting it. | ||
echo(to_unicode('\n'.join(event_column), encoding)) | ||
|
||
|
||
class NewFromString(object): | ||
|
@@ -170,10 +177,10 @@ def __init__(self, collection, conf, date_list, location=None, repeat=None): | |
'read-only'.format(collection.default_calendar_name)) | ||
sys.exit(1) | ||
if conf['default']['print_new'] == 'event': | ||
print(event.long()) | ||
echo(event.long()) | ||
elif conf['default']['print_new'] == 'path': | ||
path = collection._calnames[event.calendar].path + event.href | ||
print(path.encode(conf['locale']['encoding'])) | ||
echo(path.encode(conf['locale']['encoding'])) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is always bytes. |
||
|
||
|
||
class Interactive(object): | ||
|
@@ -186,5 +193,5 @@ def __init__(self, collection, conf): | |
description='do something') | ||
ui.start_pane( | ||
pane, pane.cleanup, | ||
program_info=u'{0} v{1}'.format(__productname__, __version__) | ||
program_info='{0} v{1}'.format(__productname__, __version__) | ||
) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't do this! Some Python 2 APIs exhibit weird behavior when fed unicode strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware of this. :(
Have you found any in khal's case? Some brief testing proved that everything worked, and tests pass, but I'm willing to look into this on a case-base-case basis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably khal doesn't even use any APIs where this matters, I just wouldn't do it as a general rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. So let's keep this around in khal's case. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really wouldn't do it... to argue my points, I've found this, but it doesn't go really into detail for all of the issues we could experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe from a readability POV (although a very weak point IMO): Because only some modules have that future-import, you can't know whether a particular string literal produces a unicode or bytestring. So we should then do this import for every module, but that means we'd have to wrap 70% of string literals with
str(...)
(because "native strings" is what most stdlib APIs actually expect)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linked website only mentions a few issues in detail, and I don't see them applying to us.
Without this import, literals are
str
, not bytestrings. With it, they become unicode strings (unicode
). And stdlib still works with unicode strings, eg:It's not merely readability either, IMHO. When writing new code, you don't need to remember to add adding
u'
to new strings - and forgetting that may cause subtle unexpected error.Personally, I don't see drawbacks with using
unicode_literals
on khal's codebase, nor any way this can come back and bite us in a forseeable future, so I'd much rather keep it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow the rest of the sentence slipped: You first have to look at the top of the file whether
unicode_literals
are imported to make sense of the literals, which is IMO dangerous while reviewing PRs.The opposite does too. Try the following code in Python 2, with
file
having nonascii characters in it:You don't have those problems when you use native string literals (by which I mean
'asdasd'
) for the format stringThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So basically my point is that changing literals everywhere (by importing
unicode_literals
) might create regressions for current Python 2 users, which are arguably the majority right now.