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

Add customizable key to disable checks customization for certain checks #550

Merged
merged 5 commits into from
Jan 21, 2025

Conversation

EMaksy
Copy link
Member

@EMaksy EMaksy commented Jan 20, 2025

Description

This PR is one of the first steps to enable users to customize checks.
It adds a new key "customization" for checks. By default all checks with values are customization.
In order to disable this behavior it needs to be explicitly set to false .
This is needed to disable checks like 3A59DC as changing it's value may cause issues.

To Do:

After merging this we need to update tlint repo reference to the new reference.

@EMaksy EMaksy added documentation Improvements or additions to documentation enhancement New feature or request labels Jan 20, 2025
@EMaksy EMaksy self-assigned this Jan 20, 2025
@coveralls
Copy link
Collaborator

coveralls commented Jan 20, 2025

Coverage Status

coverage: 97.338%. remained the same
when pulling 31535bf on add_customizable_key
into 84f0e0e on main.

@EMaksy EMaksy force-pushed the add_customizable_key branch from 0e02b70 to 7fbbec0 Compare January 20, 2025 17:05
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Eugen, just a couple of comments.

guides/check_definition.schema.json Outdated Show resolved Hide resolved
guides/specification.md Outdated Show resolved Hide resolved
guides/specification.md Outdated Show resolved Hide resolved
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EMaksy One comment.
I see that the customizable field is check wide.
This means, that you cannot make one value customizable and other not.
Did you consider adding this field to the values entries?

@EMaksy EMaksy force-pushed the add_customizable_key branch from 0319781 to 36e3bd8 Compare January 21, 2025 11:22
@EMaksy
Copy link
Member Author

EMaksy commented Jan 21, 2025

@arbulu89 just reworked that part 👍 and added additional docs

Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noice! Just a couple of adjustments.

guides/specification.md Outdated Show resolved Hide resolved
Built-in checks are considered **customizable** by **default**, so the `customizable` flag disables customization for a particular check.

The customizability flag can be set global in the check and|or in [values](#customizable-values).
When both levels are set, the **global** flag takes **precedence**, overriding any value-level customizability.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
When both levels are set, the **global** flag takes **precedence**, overriding any value-level customizability.
When customization is globally disabled for a check, that is it has been marked with `customizable: false`, it overrides any values specifics, otherwise specific values customizability is considered.

Copy link
Member Author

@EMaksy EMaksy Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split it up into two sentences. I hope this is a bit easier for the reader :)

@arbulu89
Copy link
Contributor

arbulu89 commented Jan 21, 2025

@arbulu89 just reworked that part 👍 and added additional docs

But do we need both versions (general and value specific)?
Because if the specific one overrides the other, I don't know if we really need the general one

Edit: Anyway, up to you. Having both satisfies what I commented as well

@EMaksy
Copy link
Member Author

EMaksy commented Jan 21, 2025

@arbulu89
The main reason for having the customizable field at both the top level and the value level is to provide control over whether a value is scalar or not. This allows a check to has values that are not permitted to be modified.
@nelsonkopliku please feel free to add or correct me 👍

@arbulu89
Copy link
Contributor

@arbulu89 The main reason for having the customizable field at both the top level and the value level is to provide control over whether a value is scalar or not. This allows a check to has values that are not permitted to be modified. @nelsonkopliku please feel free to add or correct me 👍

The thing to enable things that are not permitted looks like a conundrum XD
Either way, if arrays and maps are not permitted to change, but in the same moment are permitted to change...
just force explicitily to the user to put customizable: false in those entries, removing any real logic that auto-detects this I guess.

Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go with this initial version

@EMaksy EMaksy marked this pull request as ready for review January 21, 2025 13:46
@EMaksy EMaksy merged commit 8f9219d into main Jan 21, 2025
13 checks passed
@EMaksy EMaksy deleted the add_customizable_key branch January 21, 2025 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

4 participants