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 support for defining default value via environment variable #7617

Open
titouanmathis opened this issue Apr 15, 2024 · 5 comments
Open

Add support for defining default value via environment variable #7617

titouanmathis opened this issue Apr 15, 2024 · 5 comments
Labels
status: needs discussion triage needs further discussion

Comments

@titouanmathis
Copy link

What is the problem you're trying to solve?

In a shareable GitLab CI configuration, I use the custom-formatter and cache CLI arguments to improve the performance of my code quality tests and upload reports t GitLab Code Quality Report feature.

stylelint:
  variables:
    STYLELINT_ARGS: --version
    STYLELINT_FORMATTER: node_modules/stylelint-formatter-gitlab
  script:
    - npx stylelint $STYLELINT_ARGS --custom-formatter $STYLELINT_FORMATTER --cache --cache-location .stylelintcache-$CI_NODE_INDEX

This can then be used by include the configuration, and specifying the STYLELINT_ARGS variable:

include: 
  - path/to/stylelint.yml

stylelint:
  variables:
    STYLELINT_ARGS: path/to/my/src

This implementation hide the complex configurations from the final user, but also limits them in terms of what they can do.

An ideal implementation would be to define the --custom-formatter, --cache and --cache-location as environment variables to make sure that they are correctly defined, while letting the final user use the command they need.

What solution would you like to see?

Being able to define some CLI arguments as environment variables:

# Enable cache
stylelint . --cache 
STYLELINT_CACHE=true stylelint . 

# Define a custom formatter
stylelint . --custom-formatter=node_modules/formatter/index.js
STYLELINT_CUSTOM_FORMATTER=node_modules/formatter/index.js stylelint . 

# Define a custom config
stylelint . --config=path/to/config.js
STYLELINT_CONFIG=path/to/config.js stylelint . 

I think this could be implemented directly in lib/cli.mjs by setting default values when defining the flags.

I am willing to work on this feature if this is approved.

@Mouvedia Mouvedia added the status: needs discussion triage needs further discussion label Apr 15, 2024
@Mouvedia
Copy link
Contributor

What you are describing requires an user-provided config file, which already supports cache.
formatter is missing from https://github.com/stylelint/stylelint/blob/main/types/stylelint/index.d.ts#L102 though.
@ybiquitous is it an oversight?
If so should it be added to #5142?

i.e. if both properties are supported only the config is necessary, if so --config would be sufficient IMO

@ybiquitous
Copy link
Member

@titouanmathis Thanks for opening the discussion. To be honest, I don't think the support of environment variables is necessary because a JS config file can cover the use case. For example:

export default {
  cache: process.env['YOUR_ENV_CACHE'] === 'true'
};

However, as @Mouvedia commented, we haven't yet supported the formatter config property. I'm wondering why we haven't supported it until now. 🤔

If implementing the formatter property, I image the following spec:

// stylelint.config.js
import myCustomFormatter from 'my-custom-formatter';

export default {
  // A built-in formatter name
  formatter: 'string', // | 'compact' | 'json' | ...

  // Or a path to a custom formatter module
  formatter: './path/to/my-custom-formatter.js',

  // Or a custom formatter module
  formatter: myCustomFormatter,

  rules: { /* ... */ },
};
  • The --formatter or --custom-formatter CLI flags win the formatter config property.

What do you think?

@Mouvedia
Copy link
Contributor

To be honest, I don't think the support of environment variables is necessary because a JS config file can cover the use case.

👍

What do you think?

Create an issue or simply update #5142.

@ybiquitous
Copy link
Member

#5142 seems outdated. Is there a problem with rewriting this issue's title?

@ybiquitous
Copy link
Member

Or it seems better to open a new issue. I'll do that later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs discussion triage needs further discussion
Development

No branches or pull requests

3 participants