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

Add support for casts #822

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add support for casts #822

wants to merge 2 commits into from

Conversation

Kaammill
Copy link

@Kaammill Kaammill commented Mar 3, 2022

Add support for casts #710
It is my first commit therefore all feedback is more then welcome

Add support for casts
TNG#710

Signed-off-by: Kaammill  <>
@Kaammill Kaammill changed the title init commit Add support for casts Mar 3, 2022
Copy link
Member

@hankem hankem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! I've only had a very quick first look so far, but it will be easier to review whether the feature works as intended once you've added the according tests.

@@ -34,54 +34,20 @@
import com.tngtech.archunit.base.Function;
import com.tngtech.archunit.base.HasDescription;
import com.tngtech.archunit.base.Optional;
import com.tngtech.archunit.core.domain.AccessTarget;
import com.tngtech.archunit.core.domain.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for it, did not set intellij auto format,

private TypeCast(JavaCodeUnit owner, JavaClass value, int lineNumber) {
this.owner = checkNotNull(owner);
this.value = checkNotNull(value);
this.lineNumber = lineNumber;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have to store the lineNumber redundantly, but can always use sourceCodeLocation.getLineNumber().

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok i will not store lineNumber

@@ -659,6 +659,11 @@ public boolean isMetaAnnotatedWith(DescribedPredicate<? super JavaAnnotation<?>>
return members.getReferencedClassObjects();
}

@PublicAPI(usage = ACCESS)
public Set<TypeCast> getypeCast() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe getTypeCasts? (And same in JavaMembers.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so the idea is to move typeCast inside JavaMembers class? or just call getTypeCasts inside it like for example getReferencedClassObjects method?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is currently called getypeCast. As it returns a Set of TypeCast objects, I thought that a pluralized name getTypeCasts might be more appropriate.

The delegation to JavaMembers is probably fine, but I'd also rename the method there.

@codecholeric
Copy link
Collaborator

@Kaammill We have formatters for IntelliJ and Eclipse checked in here. That would automatically do the imports and other style consistently then 🙂 (also compare CONTRIBUTING where this an other things are explained. In particular also the DCO check -> you can see that this at the moment fails for your PR)

@codecholeric
Copy link
Collaborator

May I ask what the state of this PR is? 🙂

@ratoaq2
Copy link

ratoaq2 commented Mar 8, 2023

@Kaammill @codecholeric
any update on this one?
Is anything from code review missing apart from tests? I might be able to contribute if possible.

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

Successfully merging this pull request may close these issues.

4 participants