Add support for scope validation #65
Conversation
| "name": "Jonathan Garbee", | ||
| "avatar_url": "https://avatars.githubusercontent.com/u/868301?v=3", | ||
| "profile": "http://jonathan.garbee.me", | ||
| "contributions": [] |
There was a problem hiding this comment.
Don't forget to add your type of contribution by using yarn add-contributor, in this case it would be code, test, doc?
There was a problem hiding this comment.
gah, I did. It failed and I didn't catch that looking at it. At least I think I selected something... I'll get that fixed.
There was a problem hiding this comment.
Please try again and if for some reason it doesn't work, do let us know. Maybe there is another issue? Thanks!
| @@ -34,6 +34,12 @@ You can specify options in `.vcmrc` | |||
| ```js | |||
| { | |||
| "types": ["feat", "fix", "docs", "style", "refactor", "perf", "test", "chore", "revert"], // default | |||
| "scopes": { | |||
There was a problem hiding this comment.
I would maybe have this a singular. types is plural as the actual value is an array. But in this case I see this more as config for scope property of the commit message. What do you think?
| var allowedScopes = config.scopes.allowed || '*'; | ||
| var scopeRequired = config.scopes.required || false; | ||
| var scopes = scope ? scope.split(',') : []; | ||
| var validateScope = (item) => { |
There was a problem hiding this comment.
We are not ready yet I believe (correct me if I am wrong @kentcdodds) to drop support for 0.10.x. You could refactor this to es5 function?
Another idea, as this function is getting too big, would be to split all these validations. We can start by splitting the scope if (validateScopes) {...} validation and move it to lib/validation.js which could export validation functions? This should make the code easier to test in isolation as well. What do you think?
There was a problem hiding this comment.
Refactoring this is pretty easy. I just need to convert it to a standard for loop instead of using forEach on the array. Was just trying to keep it as concise as possible.
+1 to finding a way to split things up. However, I'd rather we discuss that in its own issue and PR to implement (which I'm happy working on.) That way this PR is as concise as possible to the addition being added.
There was a problem hiding this comment.
I don't think that we've ever supported 0.10.x. But I think we can drop 0.12.x too since it's no longer officially supported by NodeJS anyway.
As far as concise-ness, I'm not at all interested in making things concise. Let's focus on making things readable/maintainable for future contributors.
|
Ok, I'll get it into its own function and address the other things tomorrow. Thank you. |
|
So, dumb me forgot for a moment JS functions just go up the scope chain when you're calling a variable and it doesn't exist in the current scope. So the fat arrow wasn't even necessary, yay. Just moved the check into a function, that takes the current valid state and the scope to check as parameters. At the end, returning the valid state for the main process to continue as normal. The add-contributor method also works just fine. I just didn't press |
| @@ -21,6 +21,45 @@ var error = function() { | |||
|
|
|||
| exports.config = config; | |||
|
|
|||
| var validateScope = function(isValid, scope) { | |||
There was a problem hiding this comment.
nit: We're not doing linting (yet), but I prefer FunctionDeclarations over FunctionExpressions (so this would be: function validateScope(isValid, scope) { ... })
There was a problem hiding this comment.
And actually, because this is a utility method, I prefer that it go at the bottom of the file. Read more about my preference here
| var allowedScopes = config.scope.allowed || '*'; | ||
| var scopeRequired = config.scope.required || false; | ||
| var scopes = scope ? scope.split(',') : []; | ||
| var validateScope = function(item) { |
There was a problem hiding this comment.
Maybe use a function declaration for this one as well, also put at the bottom?
…rty now . Validation moved into its own function. Fat arrow removed since it wasn't necessary.
Garbee
force-pushed
the
garbee/filter-scopes
branch
from
4e1e10d
to
3d21903
Compare
|
So, the declarations are changed and the function is moved to the bottom with this update. However, the inner function to validate a single scope is still contained within the function instead of being on its own. Because, it is relying on the variables within the scope it is defined so it can be as simple as possible. If it were to get moved out, we'd need to pass in the I'm looking into adding more test coverage now, but that is the update on the code style. If anyone has an idea on a pattern to let the inner function get moved out without overcomplicating utilizing it then I'm all ears. |
Provides full code coverage to the new functionality.
Current coverage is 100% (diff: 100%)@@ master #65 diff @@
===================================
Files 3 3
Lines 94 121 +27
Methods 0 0
Messages 0 0
Branches 0 0
===================================
+ Hits 94 121 +27
Misses 0 0
Partials 0 0
|
|
@kentcdodds should we create a new release for this? |
|
That happens automatically ;-) Check the releases page on this repo, it's probably already done. Learn more about how that works here :) |
|
Whoops! Looks like my GitHub token and NPM token are busted... hold please. |
|
Weird, it appears travis doesn't have a build for the latest version of master... 🤔 |
|
Thank you all! If anyone reports a bug with the scope checking (or wants it modified somehow) just ping me. I don't mind hopping back in to work on it. |
|
Hey @Garbee, thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the CONTRIBUTING.md file (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :) |
|
@kentcdodds thank you very much for the link on writing open source! |
|
Alrighty, this has now been released as the latest version. Sorry for the trouble earlier! Thanks again! |
This adds support for validating allowed scopes. To do this we provide a
scopesoption object. It can contain one of four possible keys:required- Boolean to determine if a scope is requiredmultiple- Boolean to allow for multiple scopes or a singular scope onlyallowed- Array of possible values or'*'for anything.validate- Boolean to turn on validationThis does currently use an ES6 arrow function. So, that may increase the required node version. I'm not sure what the minimum supported version is. If the arrow function is a problem then I can refactor this out.