Ember cli deploy plugin control - #407

@achambers had a first look, very nice! just a couple of tiny comments so far.
I guess I'd have to see the docs to be sure of the scope of this but I assume it's still what you outlined in #349 (comment) correct?

@achambers could you close and reopen to target master ?

@ghedamat, I'm thinking I'll just force push my branch to the original branch that the original PR was based off as I'd like to keep @ef4 's PR and the discussion that happened around it.

Cool with that?

works for me,

if then you can also rebase that branch it would be golden

Yep. Of course
On Wed, 6 Jul 2016 at 21:13, Mattia Gheda [email protected] wrote:

works for me,

if then you can also rebase that branch it would be golden

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#407 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAZb1K4ZH5vPCsrbbbck_KTsee5iJ4xPks5qTAxygaJpZM4IvW7Y
.

Code and test coverage look good. I find the the chained functional style in the plugin registry a little hard to follow as a reader, but I think it's fine when paired with the comprehensive test coverage.

Hmm. Bummer. Because I did it they way to make it easier to understand. There is quite a lot of logic going on to get the list of plugins so that style was intended to be more intention revealing. To tell a story about how we get from a list of adding to a list of plugins.

Any thoughts on how I make it easier for the reader then? (Not something that's worth changing on this PR now but would like to follow up with it as this is there bulk of where the logic is so would like to make it easier to follow)

PS - Thanks for reviewing. I'll fix up that error and at that test.

@achambers not sure -- as you say, there is a lot of logic. The use of the intention-revealing method names is good. The names reveal "what" is happening. Perhaps code comments within the body of each of those methods could describe "how" it is happening?

@lukemelia I guess I was trying to write it such that you don't really need to know how....The intention was that from this list:

 var allEmberCliAddons = this._project.addons || [];
 var validAddons       = this._validAddons(allEmberCliAddons);
 var aliasMap          = this._buildAliasMap(validAddons, this._aliasConfig);
 var disabledMap       = this._buildDisabledMap(aliasMap, this._disabledConfig);
 var installedPlugins  = this._installedPlugins(validAddons, aliasMap);
 var runOrderMap       = this._buildRunOrderMap(this._runOrderConfig, aliasMap, validAddons, installedPlugins);
 var enabledPlugins    = this._applyDisabledConfig(installedPlugins, disabledMap);
 var orderedPlugins    = this._applyRunOrderConfig(enabledPlugins, runOrderMap);

you can understand the following:

1. We have a list of ember-cli addons that are installed
2. We convert that to a list of valid ember-cli-deploy addons
3. We generate the alias map
4. We generate the disabled bap
5. Based on the alias map we work out what the installed plugins are
6. We generate the run order map
7. We apply the disabled map to determine only the enabled plugins
8. We apply the run order map to determine the ordered list of plugins

I was hoping, as a reader of the code, you wouldn't really need to know how each of those things worked.

HOWEVER, I have had a couple of things on my mind in regards to things.

1. I tried to make a distinction between an installed addon (an ember-cli addon - whether it be an ecd one or not) and an installed plugin (the object returned from createDeployplugin for which there could be multiple instances for an addon based on aliases). I tried to make the distinction obvious but I'm not sure I quite got there. This still may not be obvious.

2. The return values of the _build***Map functions is not immediately obvious. I constantly found myself getting confused about what the object I was working with looked like. This is partly just the nature of things but I did toy with the idea of commenting what those functions took and the structure of what they returned.

3. The reasoning of why we need the map objects (aliasMap, disabledMap etc etc) isn't immediately obvious. It makes sense when you know but maybe this is the sort of thing that could also be documented in comments.

Are the 3 points described above more what you mean about the "how" it's working?

I'm certainly happy to make some changes to make it more understandable as this is where the bulk of the work goes and, before pulling the logic out in to this object, I continuously got confused about what was going on in the pipeline if I wasn't frequently in the code....The same could still happen with this so it's in our own interests to make it easy to understand for when we find ourselves back here.

I'll add some comments to the functions and push and see how we get on

PS - I just ran some tests with some real deploys and all seems good to me once that bug is fixed. Anyone else see any major broken things?

@achambers you summarized the challenges I had very well. Comments describing return value structure could help a bunch. For naming, I would expect the things you mentioned to be called "addon", "plugin", and "pluginInstance".

Okie doke. Will apply the above and see what we come up with. Thanks Luke

Not quite sure what the deal is with these failing tests....Can't seem to replicate on my local env.... Yu guys seen this? /cc @ghedamat @lukemelia

Thanks for pushing this over the finish line!