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

General improvments #140

Merged
merged 9 commits into from
Oct 18, 2016
Merged

General improvments #140

merged 9 commits into from
Oct 18, 2016

Conversation

dlmr
Copy link
Member

@dlmr dlmr commented Oct 17, 2016

  • CLI now use the provided --directory & fixed a problem with CLI errors
  • Fixed a problem where an option was false, when using --no-option
  • Changed so in addition to validate the configuration we also clean it
  • Added flag to infoObject that states if an object should be unmanaged
  • Improved the feedback given at validation errors and fixed a bug
  • Updated how we display object and arrays in the generated documentation
  • Added a newline in documentation generation for configuration
  • Updated the tests to match the new logic

This cleaning means that we remove old validators, descriptions, converters and similar from groups.
This allows us to define objects in our extension settings where the keys should not be verified. We can see this as something in the settings to have an actual value as an object, something that is desirable in some cases.

Related to rocjs/roc-package-web-component#8
Solved two bugs where we would not validate objects correctly using isObject.
Improved isArray and isObject validators to provide better feedback to the user.
Copy link
Member

@andreasrs andreasrs left a comment

Choose a reason for hiding this comment

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

Changes look fine, added some minor comments.

I have not had time to look into and test in detail.

unmanaged Defaults to false, Roc will not check for mismatches in projects when true.
```

If `unmanaged` is set to `true` the object will be managed in an unmanaged way where the keys on the object will not be verified by Roc. This is useful if the value for the specific property should be treated as an object.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps better written as the object will not be managed, meaning that Roc will not verify the object keys in this case.


An important thing is to make sure that a validator always returns a value for it to function correctly. It’s also encouraged that custom validators have support for generating an `infoObject` that will be used for documentation purposes, error messages and in some cases converting values. [Read more below.](#infoObject)

### Error object
For more complex validators one might need to return an error object instead of `false` / error string. This error object can be used to provide better information to user of Roc about exectly where the error occurred. The object can contain 3 properties; `key`, `value` and `message`.
Copy link
Member

Choose a reason for hiding this comment

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

exactly*

@@ -181,68 +181,35 @@ export default function runCli({
cwd: dirPath,
}).catch((error) => {
process.exitCode = error.getCode ? error.getCode() : 1;
log.small.error('A problem happened when running the Roc command');
log.small.error('An error happened when running command');
Copy link
Member

Choose a reason for hiding this comment

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

I prefer "occurred"

```
Will validate the input to make sure it’s an object consisting of the possible validator for values.
Will validate the input to make sure it’s an object consisting of the possible validator for values. Possible to provide an options object, can be the first argument if no validator is used or the second if a validator is used.
Copy link
Member

Choose a reason for hiding this comment

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

object that can be*

@@ -209,3 +209,42 @@ function updateStateMeta(name, state, extensionConfigPaths, extensionMetaPaths,

return newState.meta;
}

Copy link
Member

Choose a reason for hiding this comment

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

A lot of stuff going on in this module. Perhaps a candidate for more comments and maybe split/naming, as I do find it a bit hard to follow.

@andreasrs andreasrs merged commit f870f9b into master Oct 18, 2016
@andreasrs andreasrs deleted the general/improvements branch October 18, 2016 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants