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

feat: add analyzer for direct command handler calls #607

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

meenzen
Copy link
Contributor

@meenzen meenzen commented Jun 12, 2023

No description provided.

@alexyakunin
Copy link
Collaborator

Hi, I'll definitely have a look & merge this in a few days. Want to finish with Rpc first.

@meenzen
Copy link
Contributor Author

meenzen commented Jun 14, 2023

In the current state this should be considered a proof of concept. Unfortunately I don't have the time to properly finish this right now.

@alexyakunin
Copy link
Collaborator

Hi, still didn't get back to this - a lot of stuff on my plate, but I remember about this.

@alexyakunin
Copy link
Collaborator

alexyakunin commented Aug 16, 2023

Hi, I look up the code - couple quick things:

  • Let's use "STLC" prefix for the warning ("Stl.Commander")
  • I would also check if the call is happening on the interface vs the class. Coz technically class calls might be "ok" (e.g. base method call), + attributes are normally applied to the interface
  • Maybe add a test covering a case when such a method is called on a descendant of an interface where it is declared
  • Maybe add a test where the interface is declared in external module (i.e. is consumed from an assembly vs compiled).

I suspect there might be issues with two later tests. Overall, the attributes are inherited in Fusion - both from base classes and from the interfaces, so doing a totally correct check is kinda complicated. But covering even the primary case would be nice. That's why I wrote "maybe add a test" - i.e. I think it's good to cover at least these scenarios (they are frequent), as for anything beyond that (e.g. inspecting the whole inheritance chain), I think it's fine to do this in some other step, or even ignore this.

@alexyakunin
Copy link
Collaborator

@meenzen please check out my comment above, and sorry for delaying this.

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.

2 participants