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

Create our own config self-hosted schema #4162

Open
BraisGabin opened this issue Oct 4, 2021 · 9 comments
Open

Create our own config self-hosted schema #4162

BraisGabin opened this issue Oct 4, 2021 · 9 comments

Comments

@BraisGabin
Copy link
Member

BraisGabin commented Oct 4, 2021

We should auto generate and publish our own JSON schema to help our users write their detekt configuration file. Here there are some documentation: https://github.com/SchemaStore/schemastore/blob/master/CONTRIBUTING.md#self-hosting-schemas

#4155 for more context

@3flex 3flex changed the title Create our own JSON schema Create our own SchemaStore self-hosted schema Oct 4, 2021
@BraisGabin BraisGabin changed the title Create our own SchemaStore self-hosted schema Create our own config self-hosted schema Oct 7, 2021
@TWiStErRob
Copy link
Member

If/when this is done (or someone updates the schema in the store), it should be moved to a versioned schema, so that people can choose the right version in the IDE, here's an example from the catalog:

{
  "name": "Expo SDK",
  "description": "JSON schema for Expo SDK app manifest",
  "fileMatch": [
    "app.json"
  ],
  "url": "https://json.schemastore.org/expo-46.0.0.json",
  "versions": {
    ...
    "42.0.0": "https://json.schemastore.org/expo-42.0.0.json",
    "46.0.0": "https://json.schemastore.org/expo-46.0.0.json"
  }
},

Note: original version of Detekt schema was added in SchemaStore/schemastore#1301

@TWiStErRob
Copy link
Member

@BraisGabin I think there are two tasks here, which can be done independently:

  • generate a schema from the annotations rather than being inferred from the default config.
  • self-host the schema (version control, tag, release-process to update the catalog)

The first one doesn't sound hard, it's probably very similar to how the default config is generated?

@cortinico
Copy link
Member

@TWiStErRob Thanks for bumping this up.
Yes for 1. we should be able to extend the generate-config logic to essentially create the JSON Schema for it.

The self-hosting is something we can look into once we have the schema generation in place.
I think this is quite important, and also a bit overlooked by us, as IntelliJ pulls those schemas from SchemaStore and might show invalid warnings to users.

@TWiStErRob
Copy link
Member

I had a stab at updating the schema in the store, here's what I did:

  • git checkout v1.22.0 in the Detekt repo to be as close as possible to the release
  • rm config/detekt/detekt.yml
  • gradlew :detektGenerateConfig -> config/detekt/detekt.yml
  • Edit file and uncomment what's commented to help inference, except the weights.
  • git clone https://github.com/SchemaStore/schemastore
  • Copy detekt\detekt-core\src\main\resources\default-detekt-config.yml
    into schemastore\src\test\detekt-1.22.0\default-detekt-config.json
    via https://onlineyamltools.com/convert-yaml-to-json
  • Copy generated (non-commited) detekt/config/detekt/detekt.yml
    into schemastore\src\test\detekt-1.22.0\generated-detekt-config.json
    via https://onlineyamltools.com/convert-yaml-to-json
  • Infer schema from schemastore\src\test\detekt-1.22.0\generated-detekt-config.json
    into schemastore\src\schemas\json\detekt-1.22.0.json
    via https://www.liquid-technologies.com/online-json-to-schema-converter (Options: array rules = allow anything, make required = false)
  • Few manual edits, see commit descriptions.

This could help with defining the format of the schema in the future and also resolves some issues, like #5588

@TWiStErRob
Copy link
Member

I think this is quite important, and also a bit overlooked by us, as IntelliJ pulls those schemas from SchemaStore and might show invalid warnings to users.

Yep, see #5588, that's why I raised SchemaStore/schemastore#2623

@BraisGabin
Copy link
Member Author

Agree that this can be done in two parts. I didn't try to address this because we don't have a clear idea about how do we want our config file to look like for 2.0.

We talked about a lot of ideas but never take a decission:

  • kotlin dsl
  • yaml
  • toml

And then how are we going to handle different cases:

  • extensabilty
    • plugins should be able to add its own rules to that file
    • plugins should ve able to add tags for every rule (for example add a custom suppressors)
  • allow to configure one rule multiple times. Examples:
    • forbid some imports on production code but don't forbid then on tests
    • add different naming rules depending on an annotation (For example @Composable)
  • Nice to have: type-safety/auto-complete
  • easy to compose multiple configurations
    • we have this already but it doesn't work with lists so you can have a default list of forbidden imports and add one on a module without copying all the list.
  • And probably I'm missing things.

I think that once we take a decission on this the road to 2.0 will be way, way easier.

@TWiStErRob
Copy link
Member

TWiStErRob commented Dec 5, 2022

I really like this list, awesome summary, but I think that it's the wrong forum here in this issue. There's a present problem we're facing right now. Not in 2.0, today, and in the past years too. Bringing in 2.0 feels like scope creep at this point. Releases are going on, rules are being deprecated, new options added, things are changing, so the hand-crafted schemas get outdated and I would guess that devs are getting confused (see #5588), not even knowing why. Having a good JSON-schema means developers can take more control of their Detekt configs, without having to spend hours Google-ing navigating the documentation just to figure out trivialities like whether it's ForbiddenImport or ForbiddenImports. We have a solution right now, that works pretty well, with my PR accepted, I also added versioning so we're pretty close to giving first party support for this. It's just the case of the two steps I mentioned.

We have all the infra needed for this, right now, "just" need to connect the dots.

With first party support we can do so much better than the current inferred schema. e.g. adding the documentation right in the generated schema would bring it up in the IDE:
image
(https://docs.renovatebot.com/renovate-schema.json description and default fields are magic)

I don't think auto-complete is nice to have, DX is really important, it makes or breaks integrations. Given a choice I would always pick a product with better editor/IDE/tooling support (e.g. without Detekt IDEA plugin I would've never adopted Detekt). So I would integrate schema as first party and then maintain support level whatever refactors come in the future. Meaning if we go for DSL or something else, then maybe the detekt-idea-plugin can be extended to provide equivalent features.

So, @BraisGabin what do you think of moving "figure out v2 config" to a separate discussion and fleshing it out there, and not blocking this feature on those decisions?

@BraisGabin
Copy link
Member Author

Sure! Sorry for the off topic.

My point was more about, do we want to add this if, maybe, we are going to remove it on 2.0? And probably the answer is yes. 2.0 isn't near right now.

@TWiStErRob
Copy link
Member

TWiStErRob commented Dec 5, 2022

No worries. Yep, I think this has been ongoing for long enough to ignore the presence of 2.0 looming over us and add even if removed later. If things change in Detekt 2.0, you clearly demonstrated that we don't know how, yet, so we cannot take action on that. Move forward and do things as business as usual, while maintaining the milestone. Until there's a clear plan and timeline, I feel it's easy to use it as a deprioritization vehicle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants