Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ImportDeclaration to have getImportAsString method to get full import #660

Closed
nishant-labs opened this issue Dec 19, 2016 · 24 comments
Closed
Labels
Improvement Not a bug, but a way that JP can be be enhanced to work better.
Milestone

Comments

@nishant-labs
Copy link

ImportDeclaration class to have a getImportAsString method which will return fully qualified name of import irrespective of implementation of ImportDeclaration.

public abstract String getImportAsString(){ }

E.g.
For code import java.util.List;
output: java.util.List
For code import static java.lang.System.out;
output: java.lang.System.out
For code import java.util.*;
output: java.util.*
For code import static java.lang.System.*;
output: java.lang.System.*

@matozoid
Copy link
Contributor

Okay, one question: why would you leave "static" out, and ".*" in?

@matozoid matozoid added the Improvement Not a bug, but a way that JP can be be enhanced to work better. label Dec 19, 2016
@matozoid matozoid added this to the 3.0.0 milestone Dec 19, 2016
@nishant-labs
Copy link
Author

nishant-labs commented Dec 19, 2016

usually static is not considered as the part of import, its an access modifier, Additionally it does not provide much information about the class(except static members can be used without qualification).

The idea behind adding ".*" was to make getImportAsString method more meaningful. Once ".*" is removed from import declaration, it is no longer a valid import(in terms of class/package information)

@ftomassetti
Copy link
Member

This method could be useful, we could add it but we have just to be sure the name is very clear.
What about getImportedElement?

@matozoid
Copy link
Contributor

matozoid commented Dec 19, 2016

@ftomassetti what would you mean with "element" in this case? And certainly it would be getImportedElementAsString then? :)

edit: my point is: this part of the declaration doesn't really have a name. It's a type or a package or type and a method.

@matozoid
Copy link
Contributor

@nishantsinha-5 - maybe I'm nitpicking now, but * can officialy not be part of a qualified name since it is made up of identifiers, and * is not an identifier. (See https://docs.oracle.com/javase/specs/jls/se8/html/jls-6.html )

Alright, maybe the most important question is: what do you want to use this for? Do you simply want to print it out? Because if you want to interpret it, using the info in the subclasses is much easier.

@nishant-labs
Copy link
Author

@ftomassetti is there any specific reason/logic behind using Element

@matozoid
You are right, * is not a part of qualified name, what are you suggesting !!
I am not sure but then we can think of below implementation.
for code java.util.*
Shall we use use java.util as the value of getImportAsString and use abstract boolean isAsterisk(); method to get asterisk info.

~~"Alright, maybe the most important question is: what do you want to use this for? Do you simply want to print it out? Because if you want to interpret it, using the info in the subclasses is much easier.

Let me give information about what I am doing.

I have created a project(one of many work) where I have a list of imports(in a file with change imports from and to) and I am parsing java file then comparing the value of ImportDeclaration to the import list, if found change the value to new import.

Now I have to do heavy lifting to get those information from the ImportDeclaration in string format.
I could use toString method but it has import, ; and a trailing white-space with it.

Same goes for parsing import using JavaParser.parseImport(), where user has to write whole import declaration then only it parses it.

I would suggest if we can have JavaParser.parseImport() which will parse the import string and append other information.
e.g.
code : JavaParser.parseImport("java.util.List")
output : import java.util.List;

code : JavaParser.parseImport("java.util.*")
output : import java.util.*;

@matozoid
Copy link
Contributor

I was just going to suggest adding getPathAsString and include .* in it, after I realized that what we're talking about is a kind of path, a selector, for one of more Java classes or methods :)

toString() is even more tricky than you think: it can have comments in it.

The parse methods on JavaParser link directly to the internal parser, that's why it wants import and a ;. The nice thing is that it will parse any valid import statement, including comments and creative whitespace usage, so I'd like to keep it as it is. Is it really a big hassle to add "import" and ";"? We can make the code in CompilationUnit#addImport() more accessible if you like? There used to be a method like that in ImportDeclaration.

Thanks for being a good discussion partner, by the way.

@nishant-labs
Copy link
Author

@matozoid Thanks :)

I have never heard or seen (java docs) using "path" to denote an import.

Its not that problematic, I was just thinking of cleaner code as "import" and ";" has to be applied for all import string we parse.

@matozoid
Copy link
Contributor

Neither have I, but I have never heard any term for this part of an import :) I find getImportAsString too general, "Element" doesn't really make things any clearer, and if it's not "path," then it's your turn to suggest something again :)

It's actually a decent piece of functionality to add (or restore, really.) I think I'll also make ImportDeclaration itself visitable again. People are not responding very well to ImportDeclaration being split up into four.

@nishant-labs
Copy link
Author

I was going through Java doc

I think getQualifiedTypeName could be a better candidate for serving our purpose.

@matozoid
Copy link
Contributor

Like in the other thread, java.lang.System.out is not a type, and neither is java.util.

@matozoid
Copy link
Contributor

Also see #670

@matozoid
Copy link
Contributor

If we can't get to an agreement, I'll put "getQualifiedNamePartAsString()" in the API :)

@xiewenya
Copy link
Contributor

I encountered the same problem with different ankle. When parsing the import section, each part is parsed as class by calling visit(ClassOrInterfaceType n, Void arg).

For example, when parsing import java.util.Map , the java.util is parsed as ClassOrInterfaceType. I think it should be the package name.

@matozoid
Copy link
Contributor

matozoid commented Dec 23, 2016

@xiewenya - I just merged in the Javadoc, you can look at the Javadoc for ClassOrInterfaceType to see what the problem is.

@xiewenya
Copy link
Contributor

xiewenya commented Dec 23, 2016

@matozoid just read the javadoc, that's an awkward situation. But if there is no way to make it 100% correct, why choose the rare case( as ClassOrInterfaceType ) over the common one (as package name).

In IDE like Idea, if you declare a field as Entry , Idea will automatically change the field declaration to Map.Entry and import it as java.util.Map. So at least in Idea, the import part will always be package name plus class name.

@matozoid
Copy link
Contributor

@xiewenya you know, I wanted to say "but this is not a rare case" - but of course for an import it is the common case. I'm considering reverting the splitting up of ImportDeclaration into its four subclasses, because it seems it is not very intuitive, you're not the first one to run into trouble...

@nishant-labs
Copy link
Author

@matozoid I too was finding it difficult come into a conclusion, anyways getQualifiedNamePartAsString() looks promising.

@matozoid
Copy link
Contributor

@nishantsinha-5 would you say having four separate classes for imports instead of one is an advantage or a disadvantage?

@nishant-labs
Copy link
Author

In my opinion, It is a disadvantage for user because it created complexity while visiting ImportDeclaration types.
However I like the idea of splitting import text into types(here I would suggest to use Identifier instead of "ClassOrInterfaceType" because as per java specification each word separated by dots is called an Identifier).

But we need this getQualifiedNamePartAsString() :)

@matozoid
Copy link
Contributor

Okay, let's turn the API back into...

ImportDeclaration
   - isAsterisk (official name: "on demand")
   - isStatic
   - qualifiedNamePart

For qualified names we have Name - that supports the dots, and it has "asString()" to give you the complete name as a String.

Is that okay?

@matozoid
Copy link
Contributor

matozoid commented Dec 23, 2016

... in this case, because it will no longer clash with subclasses (because we remove them) we can also turn getQNPAS into getName.

@nishant-labs
Copy link
Author

nishant-labs commented Dec 23, 2016

@matozoid did you get chance to look at my message in gitter.im
API changes looks good and probably all issues related to this will get resolved. Thanks

@DeepSnowNeeL
Copy link
Member

DeepSnowNeeL commented Dec 24, 2016 via email

@matozoid matozoid modified the milestones: next release, 3.0.0 Dec 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Not a bug, but a way that JP can be be enhanced to work better.
Projects
None yet
Development

No branches or pull requests

5 participants