DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG� RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT <http://issues.apache.org/bugzilla/show_bug.cgi?id=32343>. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND� INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=32343 ------- Additional Comments From [EMAIL PROTECTED] 2004-11-29 03:13 ------- David, First, thank you for taking the time to look at this and give feed back. I appreciate it alot :-) > - I didn't see any JUnit test cases (not that I expected them during > prototyping). Before this is added we would need some good tests. I don't want to disagree with this as I'm a big fan of tests, but as the generated script doesn't run in a java environment they have more limited value since any generated script would still need to be tested in a browser. I developed the example webapp initially to test it and believe it is the best tool to do so. Anyway, if its agreed to include the extension, I will develop JUnit tests. > - Remove @author tags to match Validator conventions No problem - I'll take them out. > - As I understand it, validation is now hooked to onchange events which would be > highly irritating as you navigate through a form. Maybe I misunderstood this > though? No this isn't true, it becomes possible but its just an option. The Commons Validator part generates a "form validation" method which calls a bunch of "field validation" methods - so you can either validate a whole form or single field. The "customized" version of the struts tags I created generate "onchange" validation automatically - but thats just an example implementation. We would need a discussion in Struts about what features Struts provides but IMO this sort of thing should at most be available as an option. > - How does this fit into the Validator 2.0 goal of moving from the javascript > concept to a more generic script concept where the script might be python for > non-web apps using validator? Is there anything we need to do to make this > proposal generic? Maybe this is independent of that effort? Good question and I have no knowledge of how that might work. However if we provide a mechanism for configuring a ScriptRenderer maybe someone could use that to plug in their own custom renderers that generate other script languages? They would have to navigate Validator's resources in the same way. I guess one option would be to split the ScriptRender into two with it just doing the generic processing and a JavaScriptRenderer implementation with the actual javascript generation. I can do this if you think it a good idea? > ScriptRenderer > - Why is pretty output a variable? We should just simplify things and always > print well formatted javascript. I renamed this at the end of last week (but haven't fully tested a new version yet) and called it "compress" and it can now output the whole javascript on one line or in a "pretty" readable format. Appears to save around 10-20% and I believe is a useful option to have *full compression*. > - Why do some methods throw NPE instead of just returning null? Would the > caller want null in some cases? They were all things that needed to be there for it to work. Except for one static method, they could all be overriden in custom implementations if required. > - isTrue() and setBooleanConfig() should compare 'true' and 'false' as lower > case, not any case. 'TrUe' is not a valid Java boolean identifier and always > checking for the lower cased forms simplifies the usage for the client. But someone might expect "True" to work - maybe the simplest option is to always convert to lowercase when accepting input. > - Make protected methods private. This API is brand new and may need tweaking > so we should make as much private as possible until it's stable. This is the great thing about review - I'd not even considered that approach, I was focused on making everything as easily customized as possible. I'm happy to do it this way in an initial version so we could see how it works out. > - Why have static methods? From past experience we know that people will want to > override this behavior. I tried to keep them to the bare minimum - in ScriptRenderer, except for the getRenderer() method it is only a few methods that do small well defined functions (enclosing in quotes, replacing values). > - Why do we need String manipulation methods like replaceCharacters()? Wouldn't > the replacement methods on the String class work? Before 1.4 String only had replace(char, char) - the replaceCharacter() method I wrote replaces a char with a String - so that particular characters can be "escaped" the other replaceCharacters() method does multiple characters with Strings. Maybe this could be done with the Java 1.4 RegEx versions, but I'm not too good at Regex and don't think we should limit it to Java 1.4. > -Why SCRIPT_VERSION variables? The 'language' attribute isn't part of newer > specs so why bother supporting it in this new code? I'll take them out - they're not use in the Commons Validator part anyway. > - Why pass Map context to every method? This feels a bit procedural especially > in the set*Config() methods. Could we just pass the context to the renderer > constructor to wrap it? [note: I just got to the end of your feed back, its an execution context] So that someone can pass around and use an objects Validator or this extension knows nothing about. Like the message processing, the Struts implementation puts in the ServletContext and Module Prefix - that enables looking up the MessageResources later down in the getMessage() method and this Commons Validator side of the extsion knows nothing about them. I envisaged a single instance of a ScriptRenderer in, for example, a Struts Webapp - so everythings passed around in the context. > - Why do renderJavascriptStart() and renderJavascriptEnd() need hideScript and > xhtml parameters? I think XHTML rendering should be a configuration option set > in the context that affects all js output without needing to pass boolean values > to the methods. We could do the same for hideScript. Because thats the only place I could see they were relevant - they relate to rendering the HTML enclosing the Javscript, not to the javascript itself. If an implementation needed them, then theres nothing stopping them being stored in the context. These are really just convenience methods provided for implementations to use if they want, they don't relate to Commons Validator at all. > - Are there some constants in ScriptRenderer that can be made private as > implementation details of the class? Do they all need to be public? All the ones staring CONFIG_ or CONTEXT_ definetly have to be public. FIELD_VAR is used in the BaseRenderer, so could be "package friendly" and the others might be useful, but we could take the same policy as you suggested over methods - start with them private and see how it works out. > - What alternatives to using a Map context did you consider using? It seems > like we may be mixing an execution context with configuration parameters. We > used a bitwise ORing approach for configuration with UrlValidator: I didn't consider an alternative - the context has always been an execution context in my mind and thats how it developed. I got it working first storing the ServletContext, bundle, module, validator resources, locale - all things that relate to the execution. Then I added in the configuration options later. http://jakarta.apache.org/commons/validator/apidocs/org/apache/commons/validator /UrlValidator.html > Would that make sense here too? For the configuration options maybe, but its not an approach I like to employ. Thanks again for your input, I appreicate it and that you bothered to give this some time. Niall -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee. --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
