Add an option sieve for all commands#15
Conversation
Allow config:
- At the top level of `asini.json`
- In `commands.<command>` in `asini.json`
- On the command-line
Each of those supercedes previous values.
Example:
```json
{
"asini": "x.x.x",
"version": "1.2.0",
"ignore": "unrelated-package",
"commands": {
"publish": {
"ignore": "test-package"
}
}
}
```
This deprecates:
- `bootstrapConfig.ignore`
- `publishConfig.ignore`
These still work, with the _lowest_ precedence. Their use triggers a warning.
|
Ugh... no Maybe we can just drop those? Since we're not required to support everything that babel supports now? 👹 |
| } | ||
|
|
||
| get className() { | ||
| return this.constructor.name; |
There was a problem hiding this comment.
not a big fan of this, but i guess it works. ¯_(ツ)_/¯
There was a problem hiding this comment.
You would prefer each command implement a name getter?
Like:
class LsCommand extends Command {
...
get name() { return "ls" }
}That's more explicit and flexible... but it's a lot of boilerplate, too.
There was a problem hiding this comment.
something like that. yes, a bit more boilerplate, but would support classenames without Command in them and additional characters. not sure there's an advantage there, just a thought.
There was a problem hiding this comment.
and in the future, when asini is an asini repo, custom commands should be possible, and names could potentially clash. it might be nice to be explicit with the name rather than relying on the class name.
There was a problem hiding this comment.
Yeah, I see your point. Feels a little bit magic to base it on the class name.
My thought is that it's easier to get class *Command right than it is to set up a name getter. It also doesn't seem very onerous to require a naming convention for command classes. They will be relatively few, and they operate in the context of a framework.
How about I add a check for the Command suffix on class names and throw an error if a class fails to implement correctly?
There was a problem hiding this comment.
Oops, your second comment didn't show up when I was typing my response.
Clashing names would be a problem no matter which way they're specified.
How about errors on:
- Invalid class name
- Name collision
That will make it easier to catch problems in the future.
There was a problem hiding this comment.
Okay, I've got this queued up for a follow-up PR, since it works well as a standalone.
Also added an error on:
- A class that doesn't extend
Command
Will submit after this lands.
|
|
||
| _legacyOptions() { | ||
| let opts = {}; | ||
| if (this.name === "bootstrap" && (this.repository.asiniJson || {}).bootstrapConfig) { |
There was a problem hiding this comment.
i think this could be part of repository.asiniJson, always ensuring an object is returned.
There was a problem hiding this comment.
Yeah, I wavered on that. Wasn't sure if there would ever be a case where we cared whether it was even defined. I guess probably not. I'll clean this up.
| } | ||
| return opts; | ||
| } | ||
|
|
There was a problem hiding this comment.
what about something like this?
["bootstrap", "publish"].forEach((command) => {
if (this.name === command && this.repository.asiniJson[`${command}Config`]) {
logger.warn(`\`${command}Config.ignore\` is deprecated. Use \`commands.${command}.ignore\`.`);
opts.ignore = this.repository.asiniJson.[`${command}Config`].ignore;
}
});
|
|
||
| // CLI flags always override everything. | ||
| this.flags, | ||
| ); |
There was a problem hiding this comment.
i think the Flags section of the README need an entire section talking about precedence.
There was a problem hiding this comment.
Oh, yeah... good call!
Cleaner interface. No need to send the whole command object through just for one method.
gigabo
force-pushed
the
option-sieve
branch
from
bd3c440 to
e0b78b4
Compare
|
Alright, unless there are any objections I'm going to merge this in a few and get the follow up command validation PR up for review. |
2 participants
Allow config:
asini.jsoncommands.<command>inasini.jsonEach of those supercedes previous values.
Example:
{ "asini": "x.x.x", "version": "1.2.0", "ignore": "unrelated-package", "commands": { "publish": { "ignore": "test-package" } } }This deprecates:
bootstrapConfig.ignorepublishConfig.ignoreThese still work, with the lowest precedence. Their use triggers a warning.
This addresses some of the TODOs and feedback from #8.