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 cosmiconfig in config and added configuration option via package.json #101

Merged
merged 6 commits into from Jul 9, 2018

Conversation

@shashkovdanil
Copy link
Contributor

@shashkovdanil shashkovdanil commented Jul 8, 2018

No description provided.

@coveralls
Copy link

@coveralls coveralls commented Jul 8, 2018

Coverage Status

Coverage increased (+0.4%) to 94.468% when pulling 15fc64e on shashkovdanil:master into 0d98b63 on hcodes:master.

@ai
Copy link

@ai ai commented Jul 8, 2018

А зачем нам .yaspeller.json?

@shashkovdanil
Copy link
Contributor Author

@shashkovdanil shashkovdanil commented Jul 8, 2018

@ai, в README на русском языке написано вот так:

yaspeller настраивается, используя .yaspellerrc или .yaspeller.json, расположенный в корне проекта.

Если данная информация неактуальная, то уберу возможность настройки через .yaspeller.json и исправлю README.

@ai
Copy link

@ai ai commented Jul 8, 2018

А-а-а, этот формат и раньше поддерживали. Просмотрел.

У тебя тесты на Ноде 6 упали из-за отсутвия там copyFileSync.

@shashkovdanil
Copy link
Contributor Author

@shashkovdanil shashkovdanil commented Jul 8, 2018

@ai, тесты исправлены.

@ai
Copy link

@ai ai commented Jul 8, 2018

Отлично, ждём @hcodes

@@ -137,7 +137,7 @@ JSON-файл собственного словаря.
Для запуска в качестве линтера:<br/>
`npm run yaspeller`

`yaspeller` настраивается, используя `.yaspellerrc` или `.yaspeller.json`, расположенный в корне проекта.
`yaspeller` настраивается, используя `.yaspellerrc`, `.yaspeller.json`, расположенный в корне проекта, или полем `yaspeller` в `package.json`.

This comment has been minimized.

@hcodes

hcodes Jul 8, 2018
Owner

Так проще читаться будет.

yaspeller настраивается, используя JSON-файл, расположенный в корне проекта:

  • .yaspellerrc;
  • .yaspeller.json;
  • package.json, поле yaspeller.

This comment has been minimized.

@shashkovdanil

shashkovdanil Jul 9, 2018
Author Contributor

Исправил.

data = utils.loadFileAsJson(file);
}
const { config, filepath } = explorer.searchSync();
file = filepath.split('/').slice(-1)[0];

This comment has been minimized.

@hcodes

hcodes Jul 8, 2018
Owner

В отладке лучше видеть полный путь к файлу.

This comment has been minimized.

@shashkovdanil

shashkovdanil Jul 9, 2018
Author Contributor

Исправил.

config.get('test/json/wrong_prop_type.json');

const count = console.error.args.length;
assert.equal(count, 2);
});

it('get, config from package.json', function() {
fs.renameSync('./package.json', './package.json.tmp');

This comment has been minimized.

@hcodes

hcodes Jul 8, 2018
Owner

process.chdir()?

This comment has been minimized.

@shashkovdanil

shashkovdanil Jul 9, 2018
Author Contributor

Не понял зачем использовать process.chdir()

This comment has been minimized.

@hcodes

hcodes Jul 9, 2018
Owner

Зачем переименовывать, копировать и удалять package.json? Можно просто проверить в тестовой папке тестовый package.json. Сменить текущую папку, проверить package.json, восстановить текущую папку.

This comment has been minimized.

@shashkovdanil

shashkovdanil Jul 9, 2018
Author Contributor

А, ну да. Исправил.

@hcodes
Copy link
Owner

@hcodes hcodes commented Jul 8, 2018

Думаю, что можно добавить всего 3 строки и не подключать cosmiconfig вообще, раз он не понимает JSON с комментариями. Плюс, меньше зависимостей.
https://github.com/hcodes/yaspeller/blob/master/lib/config.js#L31

if (!data) {
    file = './package.json';
    const packageJson = utils.loadFileAsJson(file);
    data = packageJson && packageJson.yaspeller;
}
@shashkovdanil
Copy link
Contributor Author

@shashkovdanil shashkovdanil commented Jul 9, 2018

#101 (comment)

Вместе с package.json поддерживаются еще .yaspellerrc, .yaspeller.json. Без использования cosmiconfig придётся писать условие if для каждого файла. Также в будущем могут добавиться новые места, откуда будет браться конфиг для yaspeller. Гораздо проще их будет добавить в searchPlaces, чем добавлять новые условия для каждого файла:

const explorer = cosmiconfig('yaspeller', {
            searchPlaces: [
                'package.json',
                '.yaspellerrc',
                '.yaspeller.json'
            ]
        });

Также такой код проще читать.

@ai
Copy link

@ai ai commented Jul 9, 2018

Кстати, cosmiconfig может понимать JSON с комментариями. Просто прогоните файл, как YAML. JSON — просто подмножество YAML.

Например, все .TOOLrc файлы в cosmiconfig как раз идут через YAML ради комментариев.

@@ -1,3 +1,3 @@
[
"1" // hello

This comment has been minimized.

@hcodes

hcodes Jul 9, 2018
Owner

Смутило это удаление, думал cosmiconfig не понимает JSON с комментариями.
Точно, комментарий здесь не нужен. 👍

This comment has been minimized.

@shashkovdanil

shashkovdanil Jul 9, 2018
Author Contributor

Да, ведь файл называется no_comment

@hcodes
hcodes approved these changes Jul 9, 2018
@hcodes hcodes merged commit d386b99 into hcodes:master Jul 9, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.4%) to 94.468%
Details
@hcodes
Copy link
Owner

@hcodes hcodes commented Jul 9, 2018

Спасибо.

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.