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 option to require that fence.json files existin certain directories #45

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

Conversation

jstevans
Copy link
Contributor

Related issue: #24

@smikula
Copy link
Owner

smikula commented Nov 1, 2018

My thought with this was to make it a fence.json setting rather than a top level option. Then different subdirectories can apply their own policies.

const missingRequiredPaths = requiredPaths.filter(p => !configPathMap[p]);

if (missingRequiredPaths.length > 0) {
missingRequiredPaths.forEach(p => reportError(`Missing fence.json at ${p}`));
Copy link
Owner

Choose a reason for hiding this comment

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

reportError(Missing fence.json at ${p}) [](start = 42, length = 41)

Looks like this conflicts with my error reporting revamp. This doesn't fit the typical pattern for a validation error, but we should be able to figure out some uniform format for errors.

jstevans and others added 4 commits November 5, 2018 15:23
This is a breaking change, but should make error reporting more flexible in the future.  Now, instead of just reporting an error message string, we pass an object with various properties that may be useful to the consumer.  I've also revamped the error message to be a lot more detailed so that it's hopefully easier to track down errors when they happen.
import Config from '../types/config/Config';
import ConfigSet from '../types/ConfigSet';

interface ConfigPathPair {
Copy link
Owner

Choose a reason for hiding this comment

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

ConfigPathPair [](start = 10, length = 14)

Config has it's path as one of its properties, so you shouldn't need this structure.

@@ -0,0 +1,120 @@
import reportError from '../core/reportError';
Copy link
Owner

Choose a reason for hiding this comment

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

Can you run prettier on this file? (Settings are in the .vscode/settings.json, if you use the VSCode plugin.)

const missingRequiredPaths = Object.keys(requiredFenceMap).filter(rf => !configPathMap[rf]);

if (missingRequiredPaths.length > 0) {
missingRequiredPaths.map(p => ({key: p, values: requiredFenceMap[p]})).forEach(kvp => {
Copy link
Owner

Choose a reason for hiding this comment

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

map(p => ({key: p, values: requiredFenceMap[p]})) [](start = 29, length = 49)

Why not just look up values inside the forEach? Seems overcomplicated to iterate twice and use intermediate data structures.

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.

2 participants