Implement fluent.bundle namespace package

[spookylukey]

This the #67 branch merged onto #80

(separate PR to avoid confusing the review that is ongoing on #67).

[spookylukey]

@stasm We have a problem on this - I've checked and PyPI considers fluent-bundle and fluent.bundle to be the same package. (I attempted an upload). So we need a new package name for this, perhaps fluent.runtime. We can still use the same internal module structure, with imports like fluent.bundle as before, because the module name on the existing PyPI fluent_bundle package is fluent_bundle, so there is no clash. It is just the package name that needs to changed.

[stasm]

I didn't realize PyPI would treat these names as equal. @Pike found https://www.python.org/dev/peps/pep-0503/#normalized-names which mentions (and mandates) this behavior. I think fluent.runtime will work fine for this.

[stasm]

Because of the fluent.bundle to fluent.runtime rename, GitHub shows the comments I saved before as obsolete. I'm submitting this partial review to not lose them. I should be able to finish the review today.

[stasm]

Typo: datetime.datetime.

[stasm]

Using _get_message here means that it becomes possible to check whether a message has a given attribute. I don't think we should allow this. has_message should be used to only check of a message of the given id exists in the bundle.

In the future, we might want to add a new API for checking if a message of a given shape (i.e. with given attribute names) exists. For now, I'd suggest making has_message a bit more dumb :)

[spookylukey]

OK, will do so!

[stasm]

Please add the = after the identifier. It's required by the spec since Syntax 0.6. python-fluent supports the old syntax, too, for obscure compatibility reasons; there are parser tests to verify it. The AST for both syntaxes is the same.

[stasm]

Please update the symlink to the new directory names.

[stasm]

Nit: for consistency with CURRENCY_DISPLAY_OPTIONS, perhaps rename this to FORMAT_STYLE_OPTIONS? Or use ..._SET for both?

[spookylukey]

I'll go with _OPTIONS because I also had DATE_STYLE_OPTIONS and TIME_STYLE_OPTIONS, and _SET ties you to a set when it's possible another container object might be more appropriate.

[stasm]

Rename this to fluent_runtime?

[stasm]

One more rename here: from fluent.runtime.

[spookylukey]

These ones I was thinking of leaving i.e. currently it is just the package (and things that reflect the package name, like the directory) that are renamed to fluent.runtime. Imports are still using fluent.bundle, both in the docs and internally.

[stasm]

Hmm, I didn't understand that, then. I see what you mean: fluent.runtime the package can provide more than one module namespaced under fluent.

I think I'd be a bit surprised if pip install fluent.runtime didn't make import fluent.runtime work. It might be the choice of the period as the separator in the package name—personally, I would find it easier to understand if that name mapped exactly to the name of the module which is installed.

I don't want to block on this; I'll leave this up to you.

[stasm]

And here: from fluent.runtime.types.

[spookylukey]

Similarly here. In fact, if we wanted, we could also promote some of these user-facing things into higher level - e.g. fluent.bundle.types could go to fluent.types, which might make importing fluent_number and fluent_date a bit handier. We'd just have to add fluent.types to setup.py to enable this.

[stasm]

Wouldn't we also need to move fluent.runtime/fluent/bundle/types.py to fluent.runtime/fluent/types/__init__.py to make it a new module namespaced under fluent?

[spookylukey]

I think you're right, we'd need that too. Your other comment convinced be that it is probably better to go with fluent.runtime, it would be surprising to not have fluent.runtime importable.

[stasm]

And here as well: from fluent.runtime.types.

[stasm]

OK, I think I'm done with the review. It was a pleasure to read this code; it's very well commented and easy to follow. I'm super happy that Fluent is about to have a full implementation in Python!

I have a few minor suggestions inline which shouldn't block this PR. The high-level comment which starts in the next paragraph shouldn't block it either.

I've been thinking about how the resolver here differs from the one I wrote for fluent.js. In the JS one, I tried to stick to the following rule: all handler functions always return instances of FluentType subclasses. (Due to performance reasons, and thanks to the built-in toString and valueOf methods, native JS strings count as FluentTypes as well.) All logic is performed on these instances. At the very end, they all end up in a Pattern, where they get formatted to strings via toString.

Here, it looks like you've taken a different approach. fully_resolve is called recursively as many times as needed to get to a string. The consequence of this, IIUC, is that the resolver needs a few more handlers for types which are not AST nodes, e.g. for int and for FluentNumber. It also means that handle_variable_reference can get away with returning native types, rather than always converting them to instances of corresponding FluentTypes. This fact caught me off-guard when I was reading the code.

I'm also not entirely sure if I understand the need for FluentInt, FluentFloat, etc. Naively, I would assume that FluentNumber.value can hold any number type. match could have specialized branches for dealing with different pair of number values. What is the benefit of using types which subclass int, float and Decimal? Is it so that it's easier to write custom Fluent functions? I would be OK requiring that custom function explicitly use FluentType.value.

In fact, I would even suggest adding a FluentString type and using it across the resolver in lieu of unicode string, to further narrow down the list of return values. I don't know what the performance impact of this would be, however.

I don't want to block this PR on this discussion but I did want to start it so that you can perhaps consider if it's a follow-up material. For this PR, the current approach works great and it has all the tests to prove it. We should land it without major refactors. Thank you, @spookylukey, this is outstanding work!

[stasm]

Hmm, I didn't understand that, then. I see what you mean: fluent.runtime the package can provide more than one module namespaced under fluent.

I think I'd be a bit surprised if pip install fluent.runtime didn't make import fluent.runtime work. It might be the choice of the period as the separator in the package name—personally, I would find it easier to understand if that name mapped exactly to the name of the module which is installed.

I don't want to block on this; I'll leave this up to you.

[stasm]

Wouldn't we also need to move fluent.runtime/fluent/bundle/types.py to fluent.runtime/fluent/types/__init__.py to make it a new module namespaced under fluent?

[stasm]

(No action needed.) This highlights the lack of the resolver spec in Fluent. I'm not sure if 1.0 should be equal to 1. You might remember the discussion from projectfluent/fluent#155 (comment) about this. I hope to be able to resume this conversation in the coming months and precisely define the expected behavior of comparing Fluent's numbers. In the meantime, thanks for re-implementing the same behavior as manifested by fluent.js.

[spookylukey]

Hi @stasm - thanks so much for the review, all your encouragements and helpful comments.

For most of your comments I'll just address them directly and update the PR, only if I disagree or have something to add will I comment. So interpret silence as 'thank you I agree'!

[spookylukey]

@stasm

I've been thinking about how the resolver here differs from the one I wrote for fluent.js. In the JS one, I tried to stick to the following rule: all handler functions always return instances of FluentType subclasses. (Due to performance reasons, and thanks to the built-in toString and valueOf methods, native JS strings count as FluentTypes as well.) All logic is performed on these instances. At the very end, they all end up in a Pattern, where they get formatted to strings via toString.

To explain how I got where I did, to the best of my memory -

I started off trying to follow fluent.js quite closely (I didn't know it was you who wrote it). I think the first significant bump I hit was that in fluent.js you call toString(ctx) to finally convert, which works fine for JS because you can do that on strings themselves and they don't complain, they just ignore the extra context argument.

Translating that to Python turned out tricky (because str() does not accept an extra argument). Of course I could have added a wrapper class that had its own toString(context) method, but I think I probably missed the fact you were effectively optimizing away the FluentString wrapper by making use of the builtin toString method of strings, and so I got a bit stuck working out how to translate the code effectively.

So, I ended up going my own way and just following it through, and this is the way it turned out!

I'm also not entirely sure if I understand the need for FluentInt, FluentFloat, etc. Naively, I would assume that FluentNumber.value can hold any number type. match could have specialized branches for dealing with different pair of number values. What is the benefit of using types which subclass int, float and Decimal? Is it so that it's easier to write custom Fluent functions? I would be OK requiring that custom function explicitly use FluentType.value.

I think I was mainly thinking of custom functions which can be written assuming they are dealing with normal float/int/Decimal objects etc.,. This does have the argument of elegance for the user implementing those functions I think.

I agree that it does seem strange that the handle function ends up dealing with types other than AST nodes, and maybe we should re-visit this. However, I think it would be good to think about that after we've had a look at my compiler implementation. I haven't thought it through, but I have a suspicion that using or subclassing native types has advantages there. The compiler and resolver share all of the FluentInt etc. code, and the exceptions, but not much else. For the sake of runtime compatibility with custom functions it is important that they do share whatever we end up with for wrapper numbers etc.

[stasm]

@spookylukey, I think this is ready for landing, unless you need a bit more time. Let me know if you'd like me to merge it, or feel free to merge yourself—I've sent you an invite to collaborate on this repo :)