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

css = true behaviour is confusing #48

Closed
james1236 opened this issue Feb 21, 2023 · 9 comments · Fixed by #49
Closed

css = true behaviour is confusing #48

james1236 opened this issue Feb 21, 2023 · 9 comments · Fixed by #49

Comments

@james1236
Copy link

If you configure with the following:

names = false,
css = true

Then names gets set to true. This took me a long time to figure out, as I wanted to disable just names and nothing else. I figured that none of my options were being applied.

Having one property overwrite another is unintuitive behaviour.

This has caused confusion before

Would it be better to have individual properties like names override group properties like css?

@Akianonymus
Copy link
Collaborator

Hmm, that's a good idea, make a pr if you can, otherwise i will try in some time.

@Akianonymus
Copy link
Collaborator

Akianonymus commented Feb 22, 2023

I have gone with a different approach to solve this.

Now you can pass individual settings inside css option. css is still the most high priority option after css_fn.

If css is true, then "names", "RGB", "RRGGBB", "RRGGBBAA", "hsl_fn", "rgb_fn" will be true.

If css is a table then the values inside the css table which are false, will be false, rest will be true.

{ AARRGGBB = true, css = {names = false}, mode = "foreground" }

Here, names will be false and "RGB", "RRGGBB", "RRGGBBAA", "hsl_fn", "rgb_fn" will be true.

If css is false, then the individual settings are applied.

Same applicable for css_fn option.

By this, all the existing configuration will also not break.

@james1236
Copy link
Author

What would occur in the case of

names = false,
css = true

where it's unclear to a new user that css is overriding the seemingly completely unrelated names property?

I think priority should be narrowest to broadest

@Akianonymus
Copy link
Collaborator

What would occur in the case of

names = false,
css = true

where it's unclear to a new user that css is overriding the seemingly completely unrelated names property?

I think priority should be narrowest to broadest

In css, names are valid property.

red, black, white are valid keywords in css, no ?

@james1236
Copy link
Author

I think you are misinterpreting the original issue I raised. The behaviour where css = true overrides your setting of names = false is what caused myself and others a lot of confusion.

My faulty assumption as a new user was that css = true and names = false were unrelated settings that had no effect on eachother. I believe this assumption was reasonable and can show that others made the same assumption here.

I am suggesting that setting any narrow, individual property such as names = false should instead override any conflicting properties from groups like css = true. This way you aren't left confused by your explicit declaration (i.e names = false) being changed by what appear to be unrelated property declarations (i.e css = true) to a new user such as myself.

@Akianonymus
Copy link
Collaborator

Yeah, i understood that, also it's unclear because there is no documentation on it.

But i am in favor of the change, makes more sense, i will try to implement it. I am trying to find a way so existing configs doesn't break.

@james1236
Copy link
Author

I had a look at the code, I think this would require 3 possible values for the names property (and for any other properties that one might intend to disable individually). These values would be true, false and a third default value representing unset.

The names property would be set to unset by default, and only change if the user explicitly sets it to true or false in their config

Here would be a truth table of the outcomes of various configs:

css is true, names is true: Show names
css is true, names is unset: Show names
css is true, names is false: Don't show names

@Akianonymus
Copy link
Collaborator

Yes, sounds good except i am not changing the default values of of names, rgb and RRGGBB which is by default true. Will break user config.

Also, it's a low priority for me now, so bear with me.

Akianonymus added a commit that referenced this issue Feb 25, 2023
See #48

css is true, names is true: Show names
css is true, names is unset: Show names
css is true, names is false: Don't show names
@Akianonymus
Copy link
Collaborator

#49

Check and report

Akianonymus added a commit that referenced this issue Feb 25, 2023
See #48

css is true, names is true: Show names
css is true, names is unset: Show names
css is true, names is false: Don't show names
Akianonymus added a commit that referenced this issue Feb 25, 2023
See #48

css is true, names is true: Show names
css is true, names is unset: Show names
css is true, names is false: Don't show names
Akianonymus added a commit that referenced this issue Feb 25, 2023
See #48

css is true, names is true: Show names
css is true, names is unset: Show names
css is true, names is false: Don't show names
Akianonymus added a commit that referenced this issue Feb 25, 2023
See #48

css is true, names is true: Show names
css is true, names is unset: Show names
css is true, names is false: Don't show names
@Akianonymus Akianonymus linked a pull request Feb 25, 2023 that will close this issue
Akianonymus added a commit that referenced this issue Feb 26, 2023
See #48

css is true, names is true: Show names
css is true, names is unset: Show names
css is true, names is false: Don't show names
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 a pull request may close this issue.

2 participants