Skip to content

Implement Fluent 0.5 Private Messages #35

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 25, 2018
Merged

Implement Fluent 0.5 Private Messages #35

merged 1 commit into from
Jan 25, 2018

Conversation

zbraniecki
Copy link
Collaborator

No description provided.

@zbraniecki zbraniecki requested a review from stasm January 24, 2018 23:46
@zbraniecki
Copy link
Collaborator Author

There seems to be some weird message coming from migration tests. Not sure if it's an error or not. Would appreciate if you took a look before merging. Except of that - merge at will :)

@stasm
Copy link
Contributor

stasm commented Jan 25, 2018

What's the message? Do you mean DeprecationWarning: Please use assertRaisesRegex instead.?

@zbraniecki
Copy link
Collaborator Author

That's what I see:

projects/fluent/python-fluent  05-private ✗                                                     7h16m ◒  
▶ tox
py27 installed: six==1.11.0
py27 runtests: PYTHONHASHSEED='463181636'
py27 runtests: commands[0] | python -m unittest discover
.........sssssssssssssssssssssssssssssss....ssssssssss.sssssERROR:migrate:Migrating translations from Fluent files is not supported (privacy.ftl)
.ssssssssssssss................................................................................................................s.................
----------------------------------------------------------------------
Ran 205 tests in 0.031s

OK (skipped=61)
py27-cl installed: compare-locales==2.6.1,fluent==0.4.4,pytoml==0.1.14,six==1.11.0
py27-cl runtests: PYTHONHASHSEED='463181636'
py27-cl runtests: commands[0] | python -m unittest discover
...........ss.................ss............................ERROR:migrate:Migrating translations from Fluent files is not supported (privacy.ftl)
.s.............................................................................................................................s.................
----------------------------------------------------------------------
Ran 205 tests in 0.082s

OK (skipped=6)
py35 create: /home/zbraniecki/projects/fluent/python-fluent/.tox/py35
ERROR: InterpreterNotFound: python3.5
py36 installed: six==1.11.0
py36 runtests: PYTHONHASHSEED='463181636'
py36 runtests: commands[0] | python -m unittest discover
.....ssssssssssss.sssss/home/zbraniecki/projects/fluent/python-fluent/tests/migrate/test_context.py:432: DeprecationWarning: Please use assertRaisesRegex instead.
  with self.assertRaisesRegexp(NotSupportedError, pattern):
ERROR:migrate:Migrating translations from Fluent files is not supported (privacy.ftl)
.ssssssssssssssssssssssssssssssssssssss/home/zbraniecki/projects/fluent/python-fluent/tests/migrate/test_source.py:27: DeprecationWarning: Please use assertRaisesRegex instead.
  with self.assertRaisesRegexp(NotSupportedError, pattern):
....sssss.....................................................................................................s................................
----------------------------------------------------------------------
Ran 205 tests in 0.040s

OK (skipped=61)
________________________________________________ summary _________________________________________________
  py27: commands succeeded
  py27-cl: commands succeeded
ERROR:   py35: InterpreterNotFound: python3.5
  py36: commands succeeded

projects/fluent/python-fluent  05-private ✗                                                    7h16m ◒  ⍉
▶

@stasm - any idea why?

@stasm
Copy link
Contributor

stasm commented Jan 25, 2018

These are fine. ERROR:migrate:Migrating translations from Fluent files is not supported is expected and we even have a test for it. It probably shows up because of the default logging level.

stasm
stasm approved these changes Jan 25, 2018
View reviewed changes
fluent/syntax/ftlstream.py Outdated

return (cc >= 48 and cc <= 57) or cc == 45
cc = ord(self.current_peek()[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this taking the first char of the current peek? In other places you're not doing it.

fluent/syntax/parser.py
@@ -270,10 +277,11 @@ def get_variant_key(self, ps):
if ch is None:
raise ParseError('E0013')

if ps.is_number_start():
cc = ord(ch)
if ((cc >= 48 and cc <= 57) or cc == 45): # 0-9, -
Copy link
Contributor

Choose a reason for hiding this comment

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

ps.is_number_start sounds like the right choice here. Maybe we should fix the JS impl instead of removing it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

(I can do it in fluent.js if you decide to change it here.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know why but without this change it was breaking some tests. I didn't have time to investigate. Happy to port a patch if you write one for .js

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, let's leave it as-is for now, thanks!

@zbraniecki zbraniecki merged commit ed746d7 into projectfluent:master Jan 25, 2018
Pike
Pike reviewed Jan 25, 2018
View reviewed changes
fluent/syntax/ftlstream.py
@@ -73,20 +73,31 @@ def take_char(self, f):
return ch
return None

def is_id_start(self):
if self.ch is None:
def is_char_id_start(self, ch=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't get to actually look at this patch in full, but this one triggered my early-morning brain. ch should be an unconditional argument.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pike Pike Pike left review comments

@stasm stasm stasm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zbraniecki @stasm @Pike