Skip to content

add support to detect accesses of fields by specific methods#1105

Merged
codecholeric merged 1 commit into
TNG:mainfrom
leonardhusmann:feature/detect-reference-for-fields-in-methods
Nov 4, 2023
Merged

add support to detect accesses of fields by specific methods#1105
codecholeric merged 1 commit into
TNG:mainfrom
leonardhusmann:feature/detect-reference-for-fields-in-methods

Conversation

@leonardhusmann

@leonardhusmann leonardhusmann commented Apr 28, 2023

Copy link
Copy Markdown
Contributor

This PR adds a method to the public API of FieldsShould to check if a field has been accessed by methods matching a given predicate. This is checked by a new ArchCondition.

I've diverged a little bit from the original suggestion of this issue: I chose to stay consistent with the current API and return an ArchCondition<JavaField> to allow method chaining, e.g.

fields().that().inSomeCondition()
    .should().notBeAccessedByMethodsThat(...)
    .andShould().notBeFinal()
    .andShould()....

What do you think? Any feedback is highly appreciated :)

Resolves: #857

@leonardhusmann leonardhusmann marked this pull request as ready for review April 29, 2023 22:52
@codecholeric codecholeric force-pushed the feature/detect-reference-for-fields-in-methods branch 2 times, most recently from 020ea0e to 3cda1d4 Compare October 8, 2023 15:17
@codecholeric

codecholeric commented Oct 8, 2023

Copy link
Copy Markdown
Collaborator

Thanks a lot for adding this! 😃 And sorry that it took me so long to look at it! First of all, what do you mean by

| return an ArchCondition<JavaField> to allow method chaining

? I don't see how returning that would allow chaining as in the example? But in any case, what you did looks consistent, so thanks 🙂 Only thing I noticed is that for all the other methods we have a positive and negative form and I realized that there might be use cases where users want to combine this with a noFields start of a rule. So I added the inverted version (and adjusted the test, because I think that was a little too much mingling, i.e. it deserves it's own test). What do you think?

Independently of that, retrospectively I should have added something smarter like a generic not() function to the API 🙈 Instead of "duplicating" every method. But it's already a little inconsistent between classes and fields 😒 So maybe at some point we will release a 2.0.0 and clean up / consolidate the rules API again...

@codecholeric

codecholeric commented Oct 8, 2023

Copy link
Copy Markdown
Collaborator

Anyway, what do you think about the review changes in my commit? If everything looks good to you I'd squash everything into one commit...

@leonardhusmann

leonardhusmann commented Oct 16, 2023

Copy link
Copy Markdown
Contributor Author

Hi, thank your for your feedback 😃

First of all, what do you mean by

This is meant with regards to the initial suggestion in the issue from April '22. What I meant by this is that the arguments (DescribedPredicate) are passed as parameters and the function returns the ArchCondition. Just as you did it in your first answer in the thread of the issue 😃.

Apart from that: thank your for the review changes, they make a lot of sense :) So everything looks good to me 👍

@leonardhusmann @codecholeric
add support to detect accesses of fields by specific methods
890c3ce 
Adds `FieldsShould.{be/notBe}AccessedByMethodsThat(predicate)` to the rules API.

Issue: TNG#857
Signed-off-by: Leonard Husmann <leonard.husmann@tum.de>
@codecholeric codecholeric force-pushed the feature/detect-reference-for-fields-in-methods branch from 3cda1d4 to 890c3ce Compare November 4, 2023 16:10
@codecholeric codecholeric changed the title add support to detect references of fields in methods add support to detect accesses of fields by specific methods Nov 4, 2023
@codecholeric codecholeric self-requested a review November 4, 2023 16:15
@codecholeric

codecholeric commented Nov 4, 2023

Copy link
Copy Markdown
Collaborator

Cool, then thanks a lot again, squashed, rebased and now merged 😃

@codecholeric codecholeric merged commit e68956d into TNG:main Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@codecholeric codecholeric Awaiting requested review from codecholeric

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[How to] Detect reference for field in methods

2 participants

@leonardhusmann @codecholeric