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

Clarified "static analysis" wording in docs and logo #1008

Merged
merged 1 commit into from
Nov 4, 2021
Merged

Clarified "static analysis" wording in docs and logo #1008

merged 1 commit into from
Nov 4, 2021

Conversation

asgrim
Copy link
Contributor

@asgrim asgrim commented Nov 4, 2021

This PR updates the documentation and wording to clarify that Larastan is not actually "static analysis", since it is loading the code it is analysing. Rather than just complain on Twitter, I'd rather try to be helpful and propose changes to improve this, however, there are some additional things I can't do:

  • Repo description should be updated to Adds code analysis to Laravel improving developer productivity and code quality. or similar
  • Repo tags static-code-analysis and static-analysis should be removed, maybe add code-analysis ?
  • The logo here should be updated professionally, I'm very sorry, but I am no good at graphics editing and I'm sure this could be done to look better (see docs/logo.png - my suggestion is to replace / code static analysis / with just / code analysis / - change shown here just for demo purposes.

@szepeviktor
Copy link
Collaborator

@nunomaduro won't be happy about it.

We are working on it. Larastan will be at some point static.

@asgrim
Copy link
Contributor Author

asgrim commented Nov 4, 2021

@szepeviktor

@nunomaduro won't be happy about it.

We are working on it. Larastan will be at some point static.

I'm only going by what Nuno said regarding the dynamic loading of code:

No plans for changing that.

I am just trying to help to ensure folks are not misled by using terminology incorrectly :)

@szepeviktor
Copy link
Collaborator

szepeviktor commented Nov 4, 2021

I'm only going by what Nuno said regarding the dynamic loading of code:

AFAIK This is complicated.

@nunomaduro
Copy link
Collaborator

Thank you for suggesting this, and for opening this pull request. Yet, we don't plan to change the wording here. Keep in mind that we don't load the entire project's code. We simply "boot" an instance of the framework, so we can use the container to resolve concrete types.

I still understand your motivations @asgrim, and because of that, @canvural is currently working on removing the "boot" instance step from Larastan.

@nunomaduro nunomaduro closed this Nov 4, 2021
@nunomaduro
Copy link
Collaborator

Adjusted the readme: a244eea.

@asgrim
Copy link
Contributor Author

asgrim commented Nov 4, 2021

I still understand your motivations @asgrim, and because of that, @canvural is currently working on removing the "boot" instance step from Larastan.

Hmm, as an author of a static analysis library myself, where we have taken great pains to ensure code is never loaded - until the dynamic code loading is removed - I strongly object to this library declaring itself "static analysis" because it is misleading. I appreciate work is being done to amend it (and I also know the great undertaking that could be), but until such time, surely it would resolve confusion to remove such wording?

As it stands, Larastan is strictly NOT only a static analysis tool, and I would be severely disappointed if you don't reconsider describing this library as a "static analysis" tool :/

@asgrim
Copy link
Contributor Author

asgrim commented Nov 4, 2021

Oh, didn't see this until just after my post, sorry:

Adjusted the readme: a244eea.

Hmm, respectfully, I don't think this goes far enough; you are still shouting "Adds static analysis" etc, just with an asterisk that it's not true static analysis by definition.

@nunomaduro nunomaduro reopened this Nov 4, 2021
@nunomaduro nunomaduro merged commit 658ef46 into larastan:master Nov 4, 2021
@asgrim
Copy link
Contributor Author

asgrim commented Nov 4, 2021

@nunomaduro Thank you so much for reconsidering, but woaaaah hold your horses, the logo needs updating, it is not done properly as I mentioned in the description 🚨

@nunomaduro
Copy link
Collaborator

@asgrim It's fine. Meanwhile, @caneco if you would like to adjust this, would be cool.

@asgrim
Copy link
Contributor Author

asgrim commented Nov 4, 2021

@asgrim It's fine. Meanwhile, @caneco if you would like to adjust this, would be cool.

Thank you 🙏 appreciate it, I didn't want it to slip through the net; in hindsight I should've made the PR a "draft", since I wanted to contribute and discuss rather than have immediate fix. Again, thank you for this!

@asgrim asgrim deleted the clarify-static-analysis-wording branch November 4, 2021 09:41
@jrmajor
Copy link
Contributor

jrmajor commented Nov 4, 2021

Thank you for merging this — such a clarification would have prevented me from opening #749, because at that time it wasn't clear for me that Larastan boots my application, so I didn't expect my .env to be the issue.

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.

None yet

4 participants