Skip to content

Fix syntax errors in 'args()' AOP sample code #25357

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

Closed
wants to merge 1 commit into from

Conversation

kriegaex
Copy link
Contributor

@kriegaex kriegaex commented Jul 6, 2020

To me it looks like in your 'args()' argument binding sample pointcuts the syntax is wrong.

To me it looks like in your 'args()' argument binding sample pointcuts the syntax is wrong.
@pivotal-issuemaster
Copy link

@kriegaex Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jul 6, 2020
@pivotal-issuemaster
Copy link

@kriegaex Thank you for signing the Contributor License Agreement!

@sbrannen
Copy link
Member

sbrannen commented Jul 6, 2020

Thanks for the PR.

It turns out that the examples are actually correct, since they are using a shared common pointcut definition.

@sbrannen sbrannen closed this Jul 6, 2020
@sbrannen sbrannen added status: invalid An issue that we don't feel is valid type: documentation A documentation task and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jul 6, 2020
@sbrannen sbrannen self-assigned this Jul 6, 2020
@kriegaex
Copy link
Contributor Author

kriegaex commented Jul 6, 2020

You are right. I did not notice because I did not read the chapter above when looking for reference information about args(). IMO, this is kind of self-obfuscating. Instead of showing examples with basic syntax, you use something declared in a previous chapter without linking to it.

I am very experienced in AspectJ, also in the trickier parts of the syntax. I do not need the manual for myself, being an AspectJ guy (not really a Spring user). I sometimes open the Spring manual in order to find explanations to link to when answering questions on StackOverflow. I did not expect you to use the rather exotic case of a pointcut not declared in the same class, so you have to refer to it via fully qualified name. Thus, it looks like an oops in a method execution pointcut. You really should not expect all users to read the whole manual in one single session and then remember something from a few screens further above. How did I find the chapter? I was looking for the string "args(" on the opened web page in order to find the target chapter I could link to from StackOverflow, as I said.

Please consider to either change pointcut naming to something ugly, but obvious like pcWhateverItDoes or pointcutWhateverItDoes, then the intent would be clear. This is not "clean code" style naming, but in a reference manual you want to communicate your intent more clearly. Otherwise, explain explicitly that you are referring to a pointcut declared above and put a link to there into the explanation. If it was one long code block, the case would be different. But it is prose interspersed with little snippets. Without lots of scrolling up and down, the user's chances of understanding that part of the manual asymptotically approaches zero.

@sbrannen
Copy link
Member

sbrannen commented Jul 6, 2020

Thanks for providing such valid criticism.

I agree that it can be confusing if you don't read the whole chapter.

I think I'll just rename SystemArchitecture to SharedPointcuts, CommonPointcuts, or something similar.

What do you think about that?

By the way, you already inspired me to make the changes in 52c2ca6, before your additional comments.

@kriegaex
Copy link
Contributor Author

kriegaex commented Jul 6, 2020

I think I'll just rename SystemArchitecture to SharedPointcuts, CommonPointcuts, or something similar.

What do you think about that?

I think that is a particularly good idea, much better than mine to rename the pointcut methods themselves. SharedPointcuts would reflect the wording in the manual. Actually, the corresponding sub-chapter's headline is even "shared common pointcut definitions" which sounds kind of redundant, so maybe better not SharedCommonPointcuts. Even SystemArchitectureSharedPointcuts would be acceptable, covering both the domain language and the AOP concept. But maybe that would be kind of too verbose. Either way, the intent will be conveyed well with both of your suggestions.

Thank you very much for the swift and constructive reaction. 😀

@sbrannen
Copy link
Member

sbrannen commented Jul 6, 2020

Thank you very much for the swift and constructive reaction. 😀

You're very welcome.

Changes made in 8be2a43.

FelixFly pushed a commit to FelixFly/spring-framework that referenced this pull request Aug 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: invalid An issue that we don't feel is valid type: documentation A documentation task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants