This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
Summary: | Command-line interface (CLI) API | ||
---|---|---|---|
Product: | platform | Reporter: | Jan Chalupa <jchalupa> |
Component: | -- Other -- | Assignee: | Jaroslav Tulach <jtulach> |
Status: | RESOLVED FIXED | ||
Severity: | blocker | CC: | anebuzelsky, jglick, jlahoda, mpetras, pnejedly |
Priority: | P3 | Keywords: | API_REVIEW |
Version: | 5.x | ||
Hardware: | All | ||
OS: | All | ||
Issue Type: | ENHANCEMENT | Exception Reporter: | |
Bug Depends on: | 81195, 84800, 84802, 84803, 84804 | ||
Bug Blocks: | 57364, 69078 | ||
Attachments: |
Changes in core and utilities to make use of the sendopts module
New javadoc for the work being done on redesign_57611 of contrib/sendopts SendOpts API simplified for easy tasks |
Description
Jan Chalupa
2005-04-07 14:02:10 UTC
Created attachment 24487 [details]
Changes in core and utilities to make use of the sendopts module
Basic API is in contrib/sendopts and the changes needed in core&utilities are attached. Probably ready for next version (4.1 + 2). Btw. parts of the patch have to be merged for 4.2, as I found out that CLIOptions2 in core/src are never called. The patch here also fixes that. I'd like to get feedback on the "sendopts" API which is capable of fixing the problems that initiated this issue. First of all it provides a reasonably well justified, designed and stable API and also it enhances the possible features, so one can write a handler for list of files prefixed by no command line option. jglick, pnejedly, jlahoda agreed to look at the API and provide initial comments. Hopefully mpetras (as owner of utilities that provide --open) will have time for review as well and of course feedback of others is welcomed as well. This is supposed to be inception review to help to scope and setup goals. That is why the main review material should be the list of usecases and other architecture questions available here: http://www.netbeans.org/download/dev/javadoc/org-netbeans-modules-sendopts/architecture-summary.html The goal is to check that the usecases are complete and describe the problems. Of course you can check the whole javadoc and try the actual implementation, but only if you agree that the usecases are valid: http://www.netbeans.org/download/dev/javadoc/org-netbeans-modules-sendopts/overview-summary.html Thanks for you help with the inception review. It is appreciated as it will serve as an excellent feedback for planning of 6.0 and subsequent releases. Seems quite good to me. I have a few small comments though. Regarding the usecases: JL1: imagine that I have two options: "--folder <path>" and "--file <file-name-relative-to-folder>". Imagine that --folder is optional, defaulting to Env.getCurrentDirectory(). Imagine that the user is allowed to provide the --folder and --file options in any order - currently this seems to be quite hard to implement. Maybe the correct way is Option.pair(Option.defaultOneOf(null, folderOption), fileOption, <some-processor>), but I did not find any usecase that would cover Option.defaultOneOf and the method does not have a javadoc, so I cannot be sure. I think this might be a usefull usecase. Any opinions? JL2: imagine I have "-open-file" option, with an argument. Currently, if I assign a help to it, it gets printed as: -open-file <arg> /the help text/ I think it would be usefull if I could change the "<arg>" into something more appropriate, like "<file>". JL3: I do not fully understand the reason for usecase "Just Parse the Command Line" - who is going to call this in reality? Why do we support this usecase? (This is not to say that I am against this usecase, it is only to be sure we really want to support it.) Others: JL4: Env uses Streams (PrintStream and InputStream) - using Writers/Readers may be more appropriate, as the server and client may run on different locales with different file encodings. JL5: Help performance: issue #81195. JL6: Please document required format of the bundle name in Option.shortDescription. [jglick01] Probably the module could be autoload. [jglick02] I am unclear why there is a separate source root clisrc. It is built into the main module JAR anyway. Putting ${cp.cli} into project.properties simply obscures the (important) dependency on boot.jar. Unless there is some compelling reason why these sources need to be kept in a separate root I would suggest putting them in with the regular module sources and simplifying the project structure. This would also permit Handler and HandlerImpl to be merged, making the code more comprehensible. PN1: There's no CommandException.exitCode(String). By the availabe factories, you're forcing API users to do I18N and do it efficiently*, but they might get (already localized) error String from some other subcomponent, w/o knowledge of the defining bundle and the key. *) not really. The act of retrieving (=parsing) bundle is mostly going to be expensive enough to eclipse the time spared by not performing the key lookup and message formatting. PN2: It took me a while to understand why the options are typed and why there is a return value from the processor at all. Wouldn't the API be much simpler to understand if there was no type, no return value, and only Strings are passed internally. Most SPI usages would end up using "Void" as the type argument anyway, returning null from the processors. PN3: As you'll probably want to provide short description for every option, using Option.shortDescription wrapper looks clumsy. Generally, using {String bundle, String key} for localization is quite uncommon pattern. JL1, I'll describe the usecase. Is JL2 meant to be tcr or tca? JL3: The API was intended to be NetBeans independent as much as possible and potentially useful in standalone applications. They should then use methods in the CommandLine interface. [jglick02] This is also the reason why the clisrc is separated. I wanted the module to run and possibly compile independently on NetBeans (except openide/util). But I do not think this is needed anymore, I'll merge the two source roots to src while moving the module from contrib to core/sendopts (btw. is that acceptable location?). JL4: True about the different locale. Actually I wanted to add Locale Env.getLocale(), adding Reader and Writer is also possible (choose whether it is a TCA or TCR), but Input and Output Streams need to stay - the http://dvbcentral.sf.net streams mpeg's thru the stdout and the file might get corrupted if this was done thru Writer. JL6: I agree that a lot of documentation is missing, I did not want to invest more time to that unless it is clear that we like the API and want to include it in the platform. [jglick01] I made the module autoload. PN1 and PN3, I can fix it, just tell me how. PN2: True that most of the implementations will use Void as return type, but there is nothing wrong on that I guess. As far as I know it is a common pattern when writing generics, see for example http://download.java.net/jdk6/docs/api/javax/lang/model/element/ElementVisitor.html JL2&JL4: probably TCA's, if others agree. PN1a: CommandException.exitCode(int code, String localizedMessage) Unless you expect to do a cross-locale translation (server running en_US, client running cs_CZ, infrastructure translating server-originated exception to client locale. But that won't work with ResourceBundle argument anyway. PN2a: It's unpleasant to needlessly return null from a "Void" method. Anyway, at least document the type parameter and its most common usage ("Void", "return null") prominently (like the mentioned ElementVisitor). core/sendopts is I guess OK though I do not like the practice of putting API-providing modules into core/*. [jglick03] I am struggling to understand the sections "Complex Option Relations" and "Alternative Options". Looks rather complex to me and I cannot think of any NB code which would need it. Can these things be dropped? Surely it is best for the processor itself to handle any complicated logic relating to sets of arguments. I am concerned that the API is too large compared to the relatively simple things we really need to do with it. Also, Option.pair only works on two related options? Not three or more? Perhaps the reason you thought it necessary to have these complex composite option APIs is that the way OptionProvider works is IMHO unnatural. I would expect to see e.g. Option[] getOptions(); void handle(OptionInstance[]); where struct OptionInstance { Option option; // one of getOptions() Env env; String[] args; // may be empty, 1, even >1 for addnl args } This way I can see all the options that I claim to handle at once and decide for myself what to do with them, in which order. [jglick04] Prefer to remove CommandLine.getDefault() and make methods static, if there is no intended semantics for a CommandLine instance. [jglick05] CommandLine.parse Javadoc doesn't answer my question: what does this method do? It "parses" arguments but returns void. So...? Does it invoke handlers or not? [jglick06] Many methods in Option are undocumented. [jglick07] What is the purpose of Option.optionalArgument with no processor? [jglick08] I am as mystified as Petr by the generic type signatures. Makes API harder to understand and does not seem to add anything. Option.process in particular is too much to read. Also all type params need Javadoc. (use @param in class or method Javadoc) Created attachment 32541 [details]
New javadoc for the work being done on redesign_57611 of contrib/sendopts
Reviewers, please see the attached JavaDoc. I have implement all your requirements, as you can verify in the current version of the opinion document: http://www.netbeans.org/source/browse/*checkout*/openide/www/tutorial/reviews/opinions_57611.html?rev=1.3 and I'd like to start new round of review. Please look at the javadoc and tell me if it is acceptable. If so, I will provide patch for utilities/openfile (needs to use the API) and core/bootstrap (needs another friend) and then I'd like to integrate. Please comment by Friday 2006/08/11. I thought we had agreed that "composite" option types were unnecessary if an option processor can be given all options at once. Yet Option.*Of methods remain, and worse, they are mandatory due to the OptionProcessor constructor. This style still looks unnecessarily complex and confusing to me. I would recommend simplifying it as follows: 1. OptionProcessor to take Option[] in its constructor, representing all options it understands. 2. Option.{one,some,all}Of methods to be deleted. OptionProcessor implementations are anyway free to perform any kind of validation they like of their options and arguments, including but not limited to logic relating to the combinations of different options, possibly sensitive to actual argument values. Hardcoding this kind of logic into the API accomplishes nothing that I can see. CommandException factory methods look strange, especially as they are all named exitCode even though that is just one parameter (unhelpfully named 'e'). Prefer regular constructors with meaningful parameter names as is customary and expected for Exception subclasses. There is no compelling reason to use factory methods to construct a final class with no apparent chance of substitution in the future. Other things discussed at the meeting are not implemented, not sure if you still plan them or not: 1. Use of char rather than int for short names. 2. Removal of Option.shortDescription method and replacement with optional String shortDescription parameter to existing factory methods. Need clearer statement of what an option short description is used for. Is the line of text printed verbatim? Is the option name prepended? Is anything else printed by the infrastructure? Prefer for the infrastructure not to interfere with the help text too much, since the author of the option probably knows best how to present it, e.g. --open <file1> <file2...> Open the named files. Just need some guidelines for conventional formatting. Created attachment 33282 [details]
SendOpts API simplified for easy tasks
Reviewers here my new version that I'd like to be reviewed by Wed, Sep 13. On that day I'd like to have a final vote. JL1: Just use OptionProcessor and return two options from the getOptions method, get their values and process them as you wish JL2: Use Option.displayName JL3: External applications if they wish to use the API outside of NetBeans JL4: Streams are still there, I need them to transfer binary data. No locale support yet, but can be added compatibly when needed. JL5: done JL6: Anything still left to do? Please tell me. jglick01: Made autoload jglick02: Sources moved to one src root PN1: Now there is a way to construct CommandException with just a localized string PN2: Simplified. PN3: After discussion with reviewers it has been agreed that the lazy binding with locale is needed: http://www.netbeans.org/servlets/ReadMsg?list=nbdev&msgNo=35329 PN1a: Removed. PN2a: Removed. jglick03: Done, OptionProcessor takes Map jglick04: Not done, I still expect one might want more than one configuration of provides in the system and then the nonstatic methods will be handy. jglick05: Name changed and documented. jglick06: Documented. jglick07: No longer there. jglick08: Option class is now without generics and is simple to use. Plus answers in: http://www.netbeans.org/source/browse/*checkout*/openide/www/tutorial/reviews/opinions_57611.html?rev=1.4 Re. removal of xyzOf: http://www.netbeans.org/servlets/ReadMsg?list=nbdev&msgNo=35379 CommandException.exitCode can be transferred to constructor before integration, if it is the last problem. Approved, so Yarda will for M4: - implement TCRs/TCAs listed as blockers - increment module name to /2 - place in core/sendopts (can probably delete copy in contrib/sendopts) - use in utilities, scripting, and performance/insane modules - fix up core/bootstrap/nbproject/project.xml to not list utilities as a friend Thanks, I am working on it. Fixed all, except rewrite of performance/insane. IDE:------------------------------------------------- IDE: [18.9.06 19:15] Committing started Checking in ide/golden/public-packages.txt; /shared/data/ccvs/repository/ide/golden/public-packages.txt,v <-- public-packages.txt new revision: 1.58; previous revision: 1.57 done Checking in ide/golden/files-layout.txt; /shared/data/ccvs/repository/ide/golden/files-layout.txt,v <-- files-layout.txt new revision: 1.151; previous revision: 1.150 done Checking in ide/golden/modules.txt; /shared/data/ccvs/repository/ide/golden/modules.txt,v <-- modules.txt new revision: 1.68; previous revision: 1.67 done Checking in ide/golden/deps.txt; /shared/data/ccvs/repository/ide/golden/deps.txt,v <-- deps.txt new revision: 1.304; previous revision: 1.303 done Checking in ide/golden/friend-packages.txt; /shared/data/ccvs/repository/ide/golden/friend-packages.txt,v <-- friend-packages.txt new revision: 1.31; previous revision: 1.30 done Checking in ide/golden/cluster-deps.txt; /shared/data/ccvs/repository/ide/golden/cluster-deps.txt,v <-- cluster-deps.txt new revision: 1.51; previous revision: 1.50 done Checking in core/bootstrap/manifest.mf; /shared/data/ccvs/repository/core/bootstrap/manifest.mf,v <-- manifest.mf new revision: 1.8; previous revision: 1.7 done Checking in core/bootstrap/nbproject/project.xml; /shared/data/ccvs/repository/core/bootstrap/nbproject/project.xml,v <-- project.xml new revision: 1.13; previous revision: 1.12 done Checking in nbbuild/build.properties; /shared/data/ccvs/repository/nbbuild/build.properties,v <-- build.properties new revision: 1.404; previous revision: 1.403 done Checking in nbbuild/cluster.properties; /shared/data/ccvs/repository/nbbuild/cluster.properties,v <-- cluster.properties new revision: 1.142; previous revision: 1.141 done Checking in utilities/nbproject/project.xml; /shared/data/ccvs/repository/utilities/nbproject/project.xml,v <-- project.xml new revision: 1.20; previous revision: 1.19 done Checking in utilities/nbproject/project.properties; /shared/data/ccvs/repository/utilities/nbproject/project.properties,v <-- project.properties new revision: 1.18; previous revision: 1.17 done RCS file: /shared/data/ccvs/repository/utilities/src/META-INF/services/org.netbeans.spi.sendopts.OptionProcessor,v done Checking in utilities/src/META-INF/services/org.netbeans.spi.sendopts.OptionProcessor; /shared/data/ccvs/repository/utilities/src/META-INF/services/org.netbeans.spi.sendopts.OptionProcessor,v <-- org.netbeans.spi.sendopts.OptionProcessor initial revision: 1.1 done Removing utilities/src/META-INF/services/org.netbeans.CLIHandler; /shared/data/ccvs/repository/utilities/src/META-INF/services/org.netbeans.CLIHandler,v <-- org.netbeans.CLIHandler new revision: delete; previous revision: 1.1 done Checking in utilities/src/org/netbeans/modules/openfile/Handler.java; /shared/data/ccvs/repository/utilities/src/org/netbeans/modules/openfile/Handler.java,v <-- Handler.java new revision: 1.3; previous revision: 1.2 done Checking in utilities/src/org/netbeans/modules/openfile/Bundle.properties; /shared/data/ccvs/repository/utilities/src/org/netbeans/modules/openfile/Bundle.properties,v <-- Bundle.properties new revision: 1.39; previous revision: 1.38 done IDE: [18.9.06 19:15] Committing finished |