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

Store inspections' metadata in a file #219

Closed
mwz opened this issue Nov 23, 2018 · 12 comments
Closed

Store inspections' metadata in a file #219

mwz opened this issue Nov 23, 2018 · 12 comments

Comments

@mwz
Copy link
Contributor

mwz commented Nov 23, 2018

It would be good to have descriptions of all of the inspections available somewhere in a structured file (in resources) so it's possible to extract those easily in projects that depend on Scapegoat. Currently, the only way to extract those descriptions is by parsing the README file, which is far from ideal.
I'm thinking specifically about sonar-scala where we use Scapegoat, but I'm sure that others would also find it useful.

I'm happy to open a PR if this sounds reasonable and we can agree on the approach.

@sksamuel
Copy link
Collaborator

I think that sounds like an excellent idea.

@mwz
Copy link
Contributor Author

mwz commented Nov 25, 2018

Actually, I think that instead of storing the metadata in an external file, it might be better to add a description field to the abstract class Inspection and update each instance with a description. That would make it a single place where all the metadata is stored and once we have that in place we could then use the collection of active inspections from ScapegoatConfig.inspections and generate a list of inspections in any other format that is needed, including the inspection table for README. Perhaps we could even wrap that around an sbt task which is run on compile to keep that list always up to date.

Is that something you would be happy with @sksamuel?

@mwz
Copy link
Contributor Author

mwz commented Dec 15, 2018

Hi @sksamuel, I'd like to go ahead with implementing this, but I don't want to waste my time in case you have different opinion on how this should be done. Any thoughts would be much appreciated.

@sksamuel
Copy link
Collaborator

sksamuel commented Dec 31, 2018

I prefer the option to have it in the class itself as a field.

@carlspring
Copy link

Any update on this? :)

@mccartney
Copy link
Collaborator

I am not fully sure what metadata were meant here and what exactly do we want to add to the inspection classes here. Would someone be so kind to explain it (or give example)?
Would it still be needed/useful for derived projects?

@mwz
Copy link
Contributor Author

mwz commented Dec 11, 2019

Hi @mccartney, essentially what I'm suggesting in this issue is to add a description field to the Inspection class and pull the description of each inspection from readme into the code. This way we can make it available for any downstream projects that use Scapegoat (through ScapegoatConfig.inspections) instead of people having to hack around and parse the readme file to extract the description of each inspection.

We could also use this to automatically generate the list of inspections in the readme so it's always up-to-date and you don't have to worry about maintaining this part of the documentation.

This would still be super useful in the project I maintain, as mentioned above, it's just that I haven't been able to find the time to contribute yet, but I'm sure I'll get around doing that eventually unless someone else picks this up before me.

Does that make sense to you @mccartney?

@mccartney
Copy link
Collaborator

OK. It makes sense. Thanks for taking the time to clarify. It should be an easy change then. Let me put that to my TODO list too.

On a related note, I have also noticed (without taking Sonar into consideration) the duplication. Currently in Scapegoat we have three-to-four copies of pretty much the same:

  • the line in README.md
  • the text field of Inspection (passed as first argument in subclasses)
  • the manually constructed warning message in each warn call

@mwz
Copy link
Contributor Author

mwz commented Dec 18, 2019

Yeah, the brief description in the readme and the inspection text are the same in most cases, although they should be different in my opinion - I think the text should represent the name of the inspection and description should explain what this inspection is all about, e.g.:

SimplifyBooleanExpression:

  • text: Simplify boolean expressions
  • description: Boolean expressions such as x == false can be re-written as !x

The warning message includes some more details and is dynamic in most cases (adds a bit of context for the particular issue), so I don't think it should be removed or simplified.

@mwz
Copy link
Contributor Author

mwz commented Mar 3, 2020

Hey @mccartney, have you had a chance to look at this yet?
If not, do you mind if I get this sorted next?

I've got Scalastyle rules documented here and I'd like to do something similar with Scapegoat, so I'd be happy to raise a PR for this.

@mccartney
Copy link
Collaborator

I like the descriptions and your suggestions added in #281.
I didn't invest any time into that after my last comment here, so you are good to go.

@mwz
Copy link
Contributor Author

mwz commented Mar 12, 2020

Implemented in #281.

@mwz mwz closed this as completed Mar 12, 2020
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

No branches or pull requests

4 participants