Skip to content

feat: Move config to an external file#32

Open
dvh91 wants to merge 4 commits intomweststrate:masterfrom
dvh91:feat/config-file
Open

feat: Move config to an external file#32
dvh91 wants to merge 4 commits intomweststrate:masterfrom
dvh91:feat/config-file

Conversation

@dvh91
Copy link
Copy Markdown
Collaborator

@dvh91 dvh91 commented Jul 11, 2020

Moved relative-deps config into an external file (relative-deps.json) and still support the old way of storing the config in package.json

Motivation is that this config is personal and should not be committed to the repo.

Resolve issue #30

Comment thread README.md
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Copy link
Copy Markdown
Owner

@mweststrate mweststrate left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for doing this! Left some comments, I think the init condition isn't entirely correct yet?

Comment thread index.js
(pkg.devDependencies && pkg.devDependencies["relative-deps"]) ||
(pkg.dependencies && pkg.dependencies["relative-deps"])
)) {
if (config) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I don't think this condition is not correct, the goal of this step here is to install relative-deps itself to the package if not yet present. Probably should be !config?

Comment thread index.js
}

// the other file formats will require complex parsing and it's not in the scope of work.
const allowedConfigFiles = ['package.json', 'relativeDependenciesrc', 'relativeDependenciesrc.json'];
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Looks like this block (248 - 255) of loading and validation config should be a function that is used wherever config is loaded, so for example at line 15 as well?

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.

3 participants