-
Notifications
You must be signed in to change notification settings - Fork 63
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
Blueprintrc #117
Blueprintrc #117
Conversation
One odd thing you might note. But for blueprints purposes, the blueprint paths from the last chunk read are the ones we will check first, |
|
|
notecurrently the ProjectSettings obj instantiates a blueprint-collection at |
src/models/blueprint-collection.js
Outdated
this.settings = settings; | ||
this.setSearchPaths(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing 'settings' here was a mental disconnect for me, my first thought was that it was the ProjectSettings object, which it isn't, only the configured search paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. I'll rename that to something like rawPaths or potentialPaths
src/models/blueprint-collection.js
Outdated
// in any case we need to return an instantiated Blueprint here | ||
return path; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we keep Blueprint#load the responsibility of Blueprint so it handles the mixin logic. And move any discovery, lookup, list functionality into here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right. I'll implement it that way.
Runtime requires are definitely possible, that's essentially how the CLI is built out. We'd just need to structure the blueprint plugin modules appropriately so they provide a simple means to get at their list of blueprints. List of blueprint paths vs instantiated blueprints? To avoid potential version conflicts in the Blueprint implementation? That would give an intuitive search path for modules vs explicit paths in rc files. |
It should be easy for the entry point of the blueprint package to be a simple function that returns a list of absolute paths to all of the blueprints in it's local blueprints dir. Then BlueprintCollection can handle the loading of those paths just like any other resolved path. And I think if we have to require them, we'll definately need some token that indicates that any particular entry is an npm package. Otherwise we'll be trying to require things that may or may not exist as npm |
I feel a blueprint for creating blueprint modules is on the horizon... Agree that the modules need a simple marker in the blueprint paths so we know to require them. Might @ be confusing as it's used to scope packages, so including a scoped package would need
|
renamed source and test files changed require paths ignore references to repo, actual redux references and other projects's names changed a lot of references to .reduxrc
added npm bin definitions to allow "bp" as command
Currently only returns the paths to blueprints.
removed unneeded bin files add a project level .blueprintrc and removed it from .gitignore
increased Blueprint.js test coverage a bit
blueprintChunks count was including my ~/.blueprintrc but travis-ci didn't have that so count was different
I think this about ready to merge. @jamesr73 could you give it a review? |
renamed to .ejs until we can figure out a fix
Codecov Report
@@ Coverage Diff @@
## master #117 +/- ##
==========================================
+ Coverage 76.33% 79.42% +3.08%
==========================================
Files 21 22 +1
Lines 262 311 +49
==========================================
+ Hits 200 247 +47
- Misses 62 64 +2
Continue to review full report at Codecov.
|
src/sub-commands/config.js
Outdated
@@ -0,0 +1,47 @@ | |||
import figlet from 'figlet'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partially addresses #107
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Only minor comments/questions.
package.json
Outdated
@@ -11,7 +11,7 @@ | |||
"posttest": "npm run lint", | |||
"test:nocov": "jest --coverage=false --forceExit", | |||
"test:cov": "jest --forceExit", | |||
"test:watch": "jest --forceExit --watch --notify", | |||
"test:watch": "jest --forceExit --coverage=fals --watch --notify", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo? I was using this locally:
jest --forceExit --coverageReporters html --watch --notify
Gives a much shorter coverage summary in the jest console but also give access to a local report if you want to read it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll give that a try. I was getting frustrated when the test fails would be pushed off the screen by the full coveragae report
src/models/blueprint-collection.js
Outdated
return final; | ||
} | ||
|
||
export function parseSetting (setting) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see where parseSetting is used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah...
So that function is supposed to take the list of blueprint paths for a given .blueprintrc file and add the ./blueprints directory to the end of that list.
I believe I wrote it and then became unsure of where it should go. Let me fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it is actually being used.
it's imported in models/project-settings
and had it's name changed to parseBlueprint
. which is used in the Rc parser wrapper. It's job is to turn whatever the settings are into arrays, and to add ./blueprints
to the end of those arrays. this lets you do
It's defined in BlueprintCollection because that's the concerned party
It's called in ProjectSettings because that's where that job needs to place
I changed the name to parseBlueprintSetting for both usages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're ok with this, I'm going to merge
src/models/project-settings.js
Outdated
// if the config file list is empty, and the config is only the default | ||
// maybe save or prompt to save the default config file | ||
const startingSettings = Object.assign({}, this.defaultSettings); | ||
this.settings = rc('blueprint', startingSettings, this.args, this.myParse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does rc modify startingSettings? Might we have an issue with values in this.defaultSettings being modified since the values will be shared by startingSettings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it does. And yes we will or at least might.
default settings aren't being used right now, but we are going to need to use them. We need the default settings to have whatever settings we need to have in order to run the CLI in the complete absence of .blueprintrc files. It also needs to add our package blueprints directory.
The only real reason to maintain the integrity of the defaultSettings is to be able display just those settings, which is useful for troubleshooting complex configs.
if we keep it then change to:
startingSettings = JSON.parse(JSON.stringify(defaultSettings))
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Quick and easy. If it ever becomes problematic could migrate to a deep copy.
src/sub-commands/config.js
Outdated
console.log(prettyjson.render(this.settings.blueprints.allNames(), {}, 8)); | ||
} | ||
|
||
cliLogo () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cliLogo is duplicated in sub-commands/init.js. Perhaps move it up into SubCommand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right you are you
tweaked test:watch in package.json
some minor file formatting
👍 |
* 'master' of https://github.com/SpencerCDixon/redux-cli: Bug bp cmd (SpencerCDixon#123) Blueprintrc (SpencerCDixon#117)
This is built on top of a rename PR that's paused while we reimplement the cli
But there;s stuff to checkout.
models/project-settings
What this PR does is expand the ProjectSettings to allow it to accumulate arrays of blueprint paths, and relate them to the .blueprintrc file it came from. Other changes make is possible to track specific settings to the .blueprintrc file it came from.
models/blueprint-collection
The Other addition is models/blueprint-collection. This is a finder, loader and organizer of blueprints. It's currently instantiated by ProjectSettings, but it wouldn't take much to convice me that should actually happen in sub-command instead.
Right now it will get blueprint paths from settings, and provide a searchPaths that is an array of all of the valid blueprint paths. It also adjusts paths relative to home(~), absolute(/) and relative to the blueprintrc dir
Next up will be removing moving things from Blueprint and BlueprintCollection. Collection to be in charge of finding, loading, collecting and indexing. Blueprint doesn't have to care about anything but the blueprint at the path given