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: | Review of a new API for adding/removing entries from project classpaths | ||
---|---|---|---|
Product: | java | Reporter: | Tomas Zezula <tzezula> |
Component: | Project | Assignee: | Tomas Zezula <tzezula> |
Status: | RESOLVED FIXED | ||
Severity: | blocker | CC: | marcow, markdey, pjiricka, potingwu, ppisl, rnajman |
Priority: | P3 | Keywords: | API, API_REVIEW_FAST |
Version: | 5.x | ||
Hardware: | All | ||
OS: | All | ||
Issue Type: | DEFECT | Exception Reporter: | |
Bug Depends on: | 196455 | ||
Bug Blocks: | 60852, 73197, 73198, 74356, 75469, 93470 | ||
Attachments: |
Diff
Complete diff Diff file Diff file No semantic changes, javadoc fixes, changed variable names in implementation |
Description
Tomas Zezula
2006-04-25 10:40:33 UTC
Created attachment 30036 [details]
Diff
Fixed example of an usage of the new API by client: Project p; //Project to be changed SourceGroup sg; //SourceGroup identifying a compilation unit Library oldLibrary; //Library to be removed Library newLibrary; //Library to be added ProjectClassPathChanger c = p.getLookup().lookup (ProjectClassPathChanger.class); if (c != null) { c.removeLibrary (oldLibrary, sg, ClassPath.COMPILE); c.addLibrary (newLibrary, sg, ClassPath.COMPILE); } Suggest "ProjectClassPathModifier" rather than "...Changer". add/removeRoot should take URL, not FileObject, in case the entry doesn't yet exist. Need also patches to manifest.mf, apichanges.xml. Do you have diffs to j2seproject too? I will change the name. Agree that the URL is better than the FileObject. I will attach the j2seproject diff + tests tomorrow. AB01: in the web and EJB projects we also write some information about the libraries to project.xml apart from changing the project properties. Then the project is saved and build-impl.xml regenerated, which takes some time. This has performance issues when more items need to be added to the classpath, since project.xml would be saved for each one. Can the methods take a List of items instead of just one item? I must admit this is not really necessary now, since the only use case when we need to add multiple items at once is from within the project type, where we can (and do) cast to the project-specific impl of ProjectClassPathExtender, which contains methods accepting arrays. But if we allow the clients to mess with the project classpath the need to add/remove multiple items from outside the project may arise, so we could just support it now. Some minor stuff: AB02: ClassPathProvider.getClassPath() refers to ClassPath.COMPILE and friends as "type", not "id". For the sake of consistency let's use "type" here too. AB03: The Javadoc could give an example of a case when a method returns false and doesn't throw an exception. I suppose it can happen when e.g. adding an item which already exists, but it isn't self-evident. Created attachment 30212 [details]
Complete diff
I've attached the complete diff including tests and apichanges.xml. The new diff also fixes problems reported by Jesse. Missing SPL on J2SEProjectClassPathModifierTest.java. Y01 I can see that the change deprecates one interface and instead creates new one. I'd like to know what are the implications for client code and also what are the implications for the implementators? I would expect: project providers, stop implementing the old interface and implement the new one. Clients - continue to call both of these interfaces if you want to work with all projects! If this is correct answer, then I do not like it and I'd suggest to split client and provider API. To Andrei: I've incorporated the requirement for handling multiple entities as well as fixing the javadoc and names of parameters. I will attach the new diff as soon as the issue reported by Jarda will be resolved. Created attachment 30311 [details]
Diff file
Added the diff which solves the API/SPI separation. Another two requirements covered by this API: http://www.netbeans.org/issues/show_bug.cgi?id=73197 http://www.netbeans.org/issues/show_bug.cgi?id=73198 Imho, excelent from point of view of next future evolutions. Thanks. Now a bit of stylistic comments, but as this is a matter of personal style, take that as comments, no requests: I do not like the name Extendable much and also I do not like that it does not have any methods to modify the content - e.g. addLibrary, etc. P3 In fact I would merge the two client API classes ProjectClassPathModifier and Extendable into one. findExtendables would be the only static method, the rest would be instance methods. P5 I would change the return type from array to List<? extends Extendable>. (P10 as I am not good at inventing names) The name of the class. Why not call the merged class ExtendableProjectClassPath? There were just stylistic comments, do not take them much seriously. However what is missing is: apichange for new class in client API, there is just a change describing the addition of the class in service provider API, so please add new one or extend the one existing. s/extendable/extensible/g I don't like the Extendable (Extensible) interface in the API. Confusing (client has to iterate over all instances and do some nontrivial checks) and probably unnecessary. I would suggest removing it from the API and just exposing the methods that correspond to what the client very likely wants to do: adjust a particular source root in a particular CP mode. E.g. public static boolean addLibraries(Project project, Library[] libraries, SourceGroup group, String classPathType) throws IOException, UnsupportedOperationException; which would - return true on successful change - return false if nothing was changed - throw IOException if storing change failed - throw UnsupportedOperationException if project does not support this kind of change (adding libs, modifying this source group, or modifying this CP type) An alternate signature, even simpler for the client, and in line with what e.g. the form module would want to do: public static boolean addLibraries(FileObject referenceFile, Library[] libraries, String classPathType) throws IOException, UnsupportedOperationException; where referenceFile can be a Java source, package, or source root, and the project and source group are looked up from that (FOQ & Sources). The API without extendables is not able to keep at least some backward compatibility with ProjectClassPathExtender, which was required by Jarda. The client of ProjectClassPathExtender does not know which SourceGroup(s) the project type extends, it only knows that it extends ClassPath.COMPILE. This is the reason why the Extendable (the name may change) was introduced. For the project type providing the ProjectClassPathModifierImplementation it creates instance for each extendable classpath and its getSourceGroup returns the source group. In case when the project type provides only ProjectClassPathExtender the single Extendable which does not provide SourceGroup is created. Client may use such an extendable to add classpath elements to ClassPath.Compile. If we want to keep at least some backward compatibility the object which identifies extendable classpaths of project is needed. If you introduce a new API for clients to call then you are already expecting client code to be modified to call it. (Otherwise the client cannot work with a modified project type which only provides the new SPI.) So that client code might as well pass a SourceGroup (or FileObject), which is in fact probably _easier_ than the old code was (finding the Project with FOQ and checking its lookup), since the client probably already has a FileObject it wants to work with. The static API method would deal with looking for both the new and old SPI and acting accordingly. I don't see the point of asking clients to change their calling pattern - to something both more complicated, and semantically closer to the now-deprecated SPI. I guess the problem depends on whether the usecase shown in one javadoc in the diff is the only one we want to support right now or if there is another usecase. If we expect clients to just: ProjectClassPathModifier.Extendable extendable = null; for (ProjectClassPathModifier.Extendable e : extendables) { if (ClassPath.COMPILE.equals(e.getClassPathType()) && (e.getSourceGroup() == null || sources.equals(e.getSourceGroup()))) { extendable = e; break; } } if (extendable != null) { ProjectClassPathModifier.addLibrary (library, extendable); } Then I guess it make sense to provide some simpler client API method that will do the same as Jesse suggested. If this is the case, then imho the diff needs just two minor corrections: 1. make Extendable and methods that use it package private, 2. introduce the new method that will do exactly what is shown in the javadoc. If, in future a new requirement appears requesting more functionality, we can always make the (renamed) Extendable part of the API. That is going to be compatible change. Created attachment 30400 [details]
Diff file
I've changed the API to make it simpler while keeping backward compatibility. The Extensible changed to package private, ProjectClassPathModifier method signatures changed to take FileObject and classpath type. I was just writing this when Tomas posted the last comment. I don't like the Extendable interface either, or rather the amount of code necessary to add/remove a classpath item. If it is decided to leave Extendable (or however it will be called) in the API there should at least be some methods allowing the client to easily find an Extendable based on e.g. the SG and CP type. Jesse's suggestion of a method getting a Java source/package/root FileObject parameter makes sense to me and it elegantly covers the "extend the classpath of this file" use case. As an example, in the J2EE area we have the use case of "extending" a web module (WebModule in web/webapi) by a web framework (WebFrameworkProvider in web/webapi), which requires adding the library providing the web framework to the classpath of the web module's sources. WebModule has a getJavaSources() method returning an array of source roots, which we will pass one by one as the fileReference parameter of PCMP.addLibraries(). Another use case is in the web services support, where it seems that the JAX-WS 2.0 library is added to (and probably the JAX-RPC one removed from) the classpaths of all source groups. But that can be easily achieved by iterating over all source groups. Tomasi, you still have "classPathId" instead of "classPathType" in many places in the last diff, both in the J2SE project implementation and in the API's Javadoc ("from the classpath of the given id"). Please fix them, otherwise "id" will get copied to our project types and we will never get rid of it. "a file which classpath should be extended", "SourceGroup which classpath should be changed" -> "whose classpath". To Andrei: The new version does not expose Extensible interface, all the public API methods work with FileObject + classpath type as Jesse proposed. I've fixed the javadoc and changed the variable names to type even in the implementation. Created attachment 30403 [details]
No semantic changes, javadoc fixes, changed variable names in implementation
As far as I can tell the solution in the last diff seems to satisfy all criteria for evolution and client and provider API separation. I like it. If it also functionally works, then I agree with the integration. Thank you for fixing the variable names. The solution in the last diff supports the known use cases and it easy to use for the client. I think it can be integrated. Checking in j2seproject/src/org/netbeans/modules/java/j2seproject/J2SEProject.java; /cvs/java/j2seproject/src/org/netbeans/modules/java/j2seproject/J2SEProject.java,v <-- J2SEProject.java new revision: 1.61; previous revision: 1.60 done Checking in j2seproject/src/org/netbeans/modules/java/j2seproject/classpath/ClassPathProviderImpl.java; /cvs/java/j2seproject/src/org/netbeans/modules/java/j2seproject/classpath/ClassPathProviderImpl.java,v <-- ClassPathProviderImpl.java new revision: 1.18; previous revision: 1.17 done Checking in j2seproject/src/org/netbeans/modules/java/j2seproject/classpath/J2SEProjectClassPathExtender.java; /cvs/java/j2seproject/src/org/netbeans/modules/java/j2seproject/classpath/J2SEProjectClassPathExtender.java,v <-- J2SEProjectClassPathExtender.java new revision: 1.13; previous revision: 1.12 done RCS file: /cvs/java/j2seproject/src/org/netbeans/modules/java/j2seproject/classpath/J2SEProjectClassPathModifier.java,v done Checking in j2seproject/src/org/netbeans/modules/java/j2seproject/classpath/J2SEProjectClassPathModifier.java; /cvs/java/j2seproject/src/org/netbeans/modules/java/j2seproject/classpath/J2SEProjectClassPathModifier.java,v <-- J2SEProjectClassPathModifier.java initial revision: 1.1 done RCS file: /cvs/java/j2seproject/test/unit/src/org/netbeans/modules/java/j2seproject/classpath/J2SEProjectClassPathModifierTest.java,v done Checking in j2seproject/test/unit/src/org/netbeans/modules/java/j2seproject/classpath/J2SEProjectClassPathModifierTest.java; /cvs/java/j2seproject/test/unit/src/org/netbeans/modules/java/j2seproject/classpath/J2SEProjectClassPathModifierTest.java,v <-- J2SEProjectClassPathModifierTest.java initial revision: 1.1 done Checking in project/apichanges.xml; /cvs/java/project/apichanges.xml,v <-- apichanges.xml new revision: 1.14; previous revision: 1.13 done Checking in project/manifest.mf; /cvs/java/project/manifest.mf,v <-- manifest.mf new revision: 1.19; previous revision: 1.18 done Checking in project/nbproject/project.properties; /cvs/java/project/nbproject/project.properties,v <-- project.properties new revision: 1.21; previous revision: 1.20 done RCS file: /cvs/java/project/src/org/netbeans/api/java/project/classpath/ProjectClassPathModifier.java,v done Checking in project/src/org/netbeans/api/java/project/classpath/ProjectClassPathModifier.java; /cvs/java/project/src/org/netbeans/api/java/project/classpath/ProjectClassPathModifier.java,v <-- ProjectClassPathModifier.java initial revision: 1.1 done RCS file: /cvs/java/project/src/org/netbeans/modules/java/project/classpath/ProjectClassPathModifierAccessor.java,v done Checking in project/src/org/netbeans/modules/java/project/classpath/ProjectClassPathModifierAccessor.java; /cvs/java/project/src/org/netbeans/modules/java/project/classpath/ProjectClassPathModifierAccessor.java,v <-- ProjectClassPathModifierAccessor.java initial revision: 1.1 done Checking in project/src/org/netbeans/spi/java/project/classpath/ProjectClassPathExtender.java; /cvs/java/project/src/org/netbeans/spi/java/project/classpath/ProjectClassPathExtender.java,v <-- ProjectClassPathExtender.java new revision: 1.4; previous revision: 1.3 done RCS file: /cvs/java/project/src/org/netbeans/spi/java/project/classpath/ProjectClassPathModifierImplementation.java,v done Checking in project/src/org/netbeans/spi/java/project/classpath/ProjectClassPathModifierImplementation.java; /cvs/java/project/src/org/netbeans/spi/java/project/classpath/ProjectClassPathModifierImplementation.java,v <-- ProjectClassPathModifierImplementation.java initial revision: 1.1 done You forgot to make the new package public! It can't be used. Also typo: "projectArtfact" Also not all of the @SuppressWarnings("deprecation") are working for me: /space/src/nb_all/java/project/src/org/netbeans/api/java/project/classpath/ProjectClassPathModifier.java:29: warning: [deprecation] org.netbeans.spi.java.project.classpath.ProjectClassPathExtender in org.netbeans.spi.java.project.classpath has been deprecated import org.netbeans.spi.java.project.classpath.ProjectClassPathExtender; /space/src/nb_all/java/project/src/org/netbeans/api/java/project/classpath/ProjectClassPathModifier.java:289: warning: [deprecation] org.netbeans.spi.java.project.classpath.ProjectClassPathExtender in org.netbeans.spi.java.project.classpath has been deprecated private final ProjectClassPathExtender pcpe; /space/src/nb_all/java/project/src/org/netbeans/api/java/project/classpath/ProjectClassPathModifier.java:303: warning: [deprecation] org.netbeans.spi.java.project.classpath.ProjectClassPathExtender in org.netbeans.spi.java.project.classpath has been deprecated private Extensible (final ProjectClassPathExtender pcpe) { Checking in java/project/nbproject/project.xml; /cvs/java/project/nbproject/project.xml,v <-- project.xml new revision: 1.24; previous revision: 1.23 done Checking in java/project/src/org/netbeans/api/java/project/classpath/ProjectClassPathModifier.java; /cvs/java/project/src/org/netbeans/api/java/project/classpath/ProjectClassPathModifier.java,v <-- ProjectClassPathModifier.java new revision: 1.2; previous revision: 1.1 done Checking in ide/golden/public-packages.txt; /cvs/ide/golden/public-packages.txt,v <-- public-packages.txt new revision: 1.54; previous revision: 1.53 done There are still some deprecation warnings (2), which cannot be fixed even by annotating the whole class by @SuppressWarings ("deprecation"). |