Hi Pierre,
I took some time to read over your patch and I have a number of
comments and remarks. These are mainly due to the fact that this is
your first RIFE contribution and that you probably need to get used
to the 'spirit' of the framework. I prefer giving you constructive
criticism than implementing the changes myself because this will make
it easier for you to contribute other things in the future. Please
don't take any offense if it seems that I'm having a go at most
things you did, I'm a very difficult person when it comes to code and
rarely anything is contributed to RIFE without me making changes or
consulting with other developers (for my own additions).
Here it goes:
The code itself
---------------
* Please respect the code formatting and naming conventions that are
used throughout the entire code base, it might not be yours, but it's
the one that's adopted for the project and all code should adhere to it
* No code is accepted anymore without full javadocs for the public
classes, methods and packages. We failed to enforce this in the early
years and there are still areas that are undocumented because of this.
* There's no need to add javadoc statements like "See [EMAIL PROTECTED]
TranslatorSupport} interface". Interface methods that are implemented
without javadocs will automatically inherit the javadocs that have
been written in the interface and have a reference towards them.
* No code is accepted without tests (unit test, functional tests,
integration tests, regression tests) of all new functionalities. This
doesn't mean that you should strive for 100% coverage, but it does
mean that all expected behavior and thrown exceptions should be
covered. None of the actual i18n features that you added have tests
associated with them, I could only find some for the Encoder
refactorings you did.
The implementation
------------------
* Defensive markup encoding
If you remember well, I wasn't very enthusiast by this feature when
we discussed it here before. I couldn't quite put my finger on why,
but I do now. What you ask in fact is a more intelligent defensive
encoding functionality. The current implementation has been quickly
thrown together since it worked well enough for our purposes, but
evidently it does satisfy you. So instead of adding another
configuration option and imposing another syntax on users (to escape
the html tags), I think it's better to let the defensive encoding
feature detect when text is part of a tag or an entity and not encode
that. This shouldn't be terribly difficult to do.
You can detect tags with these regexp patterns:
<[a-z]+(?:\s+\w+\s*=\s*"[^"]*"*)*\s*/?>
</[a-z]+>
and entities with this regexp pattern:
&\w+;
I think that everything outside those can safely be encoded.
Maybe some special handling is needed for script tags though so that
the text in between those is always unescaped.
Anyway, I think this should better be left for a second stage and
that the l18n is first focused on the dynamic provision of locales. I
would thus not at all create an encoder package, nor perform the
refactorings you did.
* The I18n class
This seems to be too hardcoded and linked to a SpringWeb participant.
I don't like this very much since Spring should merely be a one
possible IoC reference factory. I haven't looked at all the
interactions, but please try to leverage the HierarchicalProperties
support of RIFE that is present throughout the whole application
(from Rep, to participants, to elements): http://rifers.org/wiki/
display/RIFE/IoC+properties+support+inside+the+repository
* Additional ElementSupport methods
I would leave out all methods that simply delegate to getTranslator,
but that's just something minor.
However, I don't feel comfortable with the getTemplate
(TemplateFactory factory, String name, String encoding,
TemplateTransformer transformer) method and sibling methods that you
added. I don't particularly like the changes you performed throughout
the rest of the framework to delegate the instantiation of all engine
template types to an element. It would be best to perform the
resourcebundle addition code in the EngineTemplateInitializer class:
http://rifers.org:8088/viewrep/rifers/rife/trunk/src/framework/com/
uwyn/rife/engine/EngineTemplateInitializer.java?r=HEAD
As you see, it's possible that these templates are instantiated
without elements, so don't make the presence of an element mandatory,
things will break.
This is about everything what I saw during my first cursory look.
I'll go over it again tomorrow and report if I see other things.
Please only focus on the LocaleProvider stuff for now and do the
encoding modifications in a second stage. It's best to commit these
features in different changesets.
Thanks a lot for your contributions and your involvement.
Happy new-year!
Geert
On 30-dec-05, at 15:06, Raoul Pierre wrote:
Geert Bevin a écrit :
Is everything in the new i18n package tested?
Yes but EncoderSql / Xhtml / Xml
And everything works as current Rife if there is no participant for
the i18nContext.xml file.
The main API change is the removed TemplateEncoder interface: there
is now the com.uwyn.rife.i18n.Encoder.
The idea is:
- Encoder is accessible from the Element level;
- but direct access must be avoided: Translator hides it.
Note. There are two types of encodage: charset and markup. And in
the two cases, "encode" words are used. It can be confusing. Any
better idea for one of them?
Pierre
_______________________________________________
Rife-users mailing list
[email protected]
http://www.uwyn.com/mailman/listinfo/rife-users
--
Geert Bevin Uwyn bvba
"Use what you need" Avenue de Scailmont 34
http://www.uwyn.com 7170 Manage, Belgium
gbevin[remove] at uwyn dot com Tel +32 64 84 80 03
PGP Fingerprint : 4E21 6399 CD9E A384 6619 719A C8F4 D40D 309F D6A9
Public PGP key : available at servers pgp.mit.edu, wwwkeys.pgp.net
_______________________________________________
Rife-users mailing list
[email protected]
http://www.uwyn.com/mailman/listinfo/rife-users
