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

feat(get-config): add support for defining plugins as an object #2154

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

XC-
Copy link
Contributor

@XC- XC- commented Sep 27, 2021

This implementation allows the plugins to be defined as an object instead of an array. The benefit
is readability and usability, especially when using yaml format, where empty objects can be left
out. In Javascript, the benefit is more limited since it is necessary to put in the empty objects,
but even then the configuration should be more intuitive. This is implemented in backwards
compatible way, where the original format is still used internally, so nothing should break.

@XC-
Copy link
Contributor Author

XC- commented Sep 27, 2021

Even though this is still a work in progress, or rather, especially because this is wip, I would appreciate thoughts from @travi , @gr2m and/or @pvdlg .

edit: I was just blind with the tests x(

This implementation allows the plugins to be defined as an object instead of an array. The benefit
is readability and usability, especially when using yaml format, where empty objects can be left
out. In Javascript, the benefit is more limited since it is necessary to put in the empty objects,
but even then the configuration should be more intuitive. This is implemented in backwards
compatible way, where the original format is still used internally, so nothing should break.
@gr2m
Copy link
Member

gr2m commented Sep 27, 2021

I'd usually start by creating an issue to have a discussion before sending a PR :)

Can you update the docs to account for your change? That's the best starting point to have a discussion

@XC-
Copy link
Contributor Author

XC- commented Sep 28, 2021

I'd usually start by creating an issue to have a discussion before sending a PR :)

Yea, I'm way too used to thinking through PoCs so, this kinda just happened ^^;

Can you update the docs to account for your change? That's the best starting point to have a discussion

Will do. I'll update them later today after work.

…S locale

Added tests to check different situations that the normalization function could in the worst case
encounter. Also added LANG=en_US.UTF-8 in front of the test scripts to guarantee that the tests are
run using English language locally. Without the variable, yargs will use whatever is set to LANG and
some tests will fail.
@XC-
Copy link
Contributor Author

XC- commented Sep 28, 2021

@gr2m I added some tests and documentation + examples for the new feature. There was an problem with the tests, where some of the yargs output was in Finnish because of my LANG variable, so I added LANG=en_US.UTF-8 to fix it. I also encountered one possibly flaky test that can occasionally fail (though not often), I'll open a issue for that in case there isn't one already.

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 this pull request may close these issues.

None yet

2 participants