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

Symbolsolver import order #2521

Merged
merged 3 commits into from Feb 18, 2020
Merged

Conversation

@iTakeshi
Copy link
Contributor

iTakeshi commented Feb 11, 2020

This PR solves two problems.

1: 896e8da

A single-type-import (JLS 7.5.1) and a single-static-import(JLS 7.5.3) can import same name into a compilation unit, when the static-imported name doesn't refer a type (e.g., enum member, etc.).
In such cases, those static imports can just be ignored and other imports should be evaluated.
(This commit also includes minor refactoring around nested if)

2: 2e71583

According to JLS 7.5.2 and JLS 6.4.1, asterisk imports should be evaluated after package members.

@iTakeshi

This comment has been minimized.

Copy link
Contributor Author

iTakeshi commented Feb 11, 2020

The travis build failed seemingly because of a network connection error. Could you trigger to rebuild? (i.e., rebuild request needs a write-access to the repo)
Thanks for your help! (Cc: maybe @matozoid )

@iTakeshi iTakeshi force-pushed the iTakeshi:symbolsolver-import-order branch from 2e71583 to a26e26e Feb 14, 2020
@iTakeshi

This comment has been minimized.

Copy link
Contributor Author

iTakeshi commented Feb 14, 2020

Thanks @MysterAitch for your review.
(I'm sorry that your comment got invisible due to my force push... I didn't know that.)

As you commented, the assertion messages were not descriptive enough for the test cases.
I think it was because of the class name Name is highly confusing, thus I renamed those classes to just class A.
Also I revised the assertion messages based on your suggestion.

@iTakeshi

This comment has been minimized.

Copy link
Contributor Author

iTakeshi commented Feb 14, 2020

@matozoid @MysterAitch
Also a management question: On revising PRs, should I just add new commits on top of original PR commits, or should I force-push them just like I did for this PR?
Personally I don't like the commit log gets dirty because of those revising commits, thus I prefer force-pushing the revised commits.

If you could tell me your preference for this project, I will follow the guideline.

@matozoid

This comment has been minimized.

Copy link
Member

matozoid commented Feb 18, 2020

@iTakeshi please no force pushing if it confuses github.

@matozoid matozoid merged commit e851256 into javaparser:master Feb 18, 2020
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 64.373%
Details
@matozoid

This comment has been minimized.

Copy link
Member

matozoid commented Feb 18, 2020

... and thanks for the PR! :-)

@matozoid matozoid added this to the next release milestone Feb 18, 2020
@iTakeshi iTakeshi deleted the iTakeshi:symbolsolver-import-order branch Feb 18, 2020
@iTakeshi

This comment has been minimized.

Copy link
Contributor Author

iTakeshi commented Feb 18, 2020

please no force pushing if it confuses github.

Understood, I will add commits except for when there are no side effects. Thanks for the guideline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.