Bug 201254 - API review: Added GeneratorUtilities.addImports method
API review: Added GeneratorUtilities.addImports method
Status: RESOLVED FIXED
Product: java
Classification: Unclassified
Component: Source
7.1
All All
: P3 (vote)
: 7.1
Assigned To: Dusan Balek
issues@java
: API, API_REVIEW_FAST
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-23 15:16 UTC by Dusan Balek
Modified: 2011-08-29 16:24 UTC (History)
1 user (show)

See Also:
Issue Type: TASK
:


Attachments
Proposed patch (23.96 KB, patch)
2011-08-23 15:27 UTC, Dusan Balek
Details | Diff
Patch with the Javadoc added (26.39 KB, patch)
2011-08-24 08:43 UTC, Dusan Balek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dusan Balek 2011-08-23 15:16:08 UTC
Added GeneratorUtilities.addImports method to add import statements for given elements to a compilation unit in a way that complies with the rules specified in CodeStyle. Also, CodeStyle.importInnerClasses, CodeStyle.getImportGroups, and CodeStyle.separateImportGroups are added to specify the rules.
Comment 1 Dusan Balek 2011-08-23 15:27:18 UTC
Created attachment 110162 [details]
Proposed patch
Comment 2 Jesse Glick 2011-08-23 15:34:30 UTC
Missing Javadoc for additions in CodeStyle.

Why would you use addImports rather than the existing fixImports, which seems more convenient? What is the use case?
Comment 3 Dusan Balek 2011-08-24 08:43:17 UTC
Created attachment 110176 [details]
Patch with the Javadoc added
Comment 4 Dusan Balek 2011-08-24 09:29:53 UTC
GeneratorUtilities.addImports is not introduced to replace the FixImports or any other higher-level feature managing imports, but rather to fix the current situation where the FixImports and Hints use the non-API JavaFixAllImports.addImports method to add necessary import statements to a compilation unit which is the exact copy of the non-API SourceUtils.addImport method used by the Code Completion and Code Templates.

GeneratorUtilities.addImports is introduced as a low-level API method that should be used by ALL higher-level editor features (FixImports, Hints, Code Completion, Code Templates, etc) to add necessary import statements into a compilation unit. In contrast to the original non-API methods, it should follow the rules specified in CodeStyle (like whether to use single class imports or package imports, whether and how to group imports, how to order the import groups, etc).
Comment 5 Jesse Glick 2011-08-24 17:56:43 UTC
Perhaps my question was not clear, since I gave the wrong method name originally.

There is already a public API GeneratorUtilities.importFQNs which (as far as I can tell) works fine to add import statements to the source file, usable from various features (editor hint fixes, etc.) which insert code blocks defined using TreeMaker with fully-qualified identifiers.

So is there something wrong with importFQNs that requires a new method? (addImports looks like it would be more work to use correctly - you not only need to enumerate everything you want imported, which importFQNs would handle automatically, but you need to rewrite the entire CompilationUnitTree, presumably using TreeUtilities.translate.)

Or are the proposed uses for addImports of a different nature than the existing uses of importFQNs?
Comment 6 Jan Lahoda 2011-08-25 08:33:14 UTC
There is nothing wrong with TreeMaker.QualIdent/Type and GeneratorUtilities.importFQNs (all internally based on the QualIdent). In fact, these methods methods are preferred over SourceUtils.resolveImport or the new GeneratorUtilities.addImports and should be used by vast majority of features. The QualIdent-based methods should always do the "right thing", so that the correct class is referred in the source. There are, however, some features that cannot use the QualIdent-based methods, like the add import hint (where the user explicitly said that a particular import should be added). The SourceUtils.resolveImport and GeneratorUtilities.addImports are intended only for these special cases.

I think this should be clarified in the javadoc of these methods. What about:
<p><strong>Use TreeMaker.QualIdent, TreeMaker.Type or GeneratorUtilities.importFQNs instead of this method if possible. These methods will correctly resolve imports according to the user's preferences.</strong></p>

I also suggest to deprecate the TreeMaker.{add|insert}CompUnitImport, as I see no valid use case for these methods, even though there are several uses of them across the codebase.
Comment 7 Jesse Glick 2011-08-25 11:09:23 UTC
(In reply to comment #6)
> <p><strong>Use TreeMaker.QualIdent, TreeMaker.Type or
> GeneratorUtilities.importFQNs instead of this method if possible. These methods
> will correctly resolve imports according to the user's
> preferences.</strong></p>

Thanks, that would certainly make the situation clearer.

> deprecate the TreeMaker.{add|insert}CompUnitImport

Perhaps this should be left alone as a low-level structure manipulation API, with a clear warning in the Javadoc that typical code should be using addImports or importFQNs instead.
Comment 8 Dusan Balek 2011-08-29 16:24:46 UTC
Integrated into in jet-main.

http://hg.netbeans.org/jet-main/rev/c604329f2888


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo