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

Recursively look for oc.json, starting from componentsDir #313

Merged
merged 7 commits into from
Oct 24, 2016
Merged

Conversation

matteofigus
Copy link
Member

Fixes #219

@matteofigus matteofigus removed the wip label Oct 21, 2016
@matteofigus matteofigus changed the title [wip] Recursively look for oc.json, starting from componentsDir Recursively look for oc.json, starting from componentsDir Oct 21, 2016
Copy link
Member

@mattiaerre mattiaerre left a comment

Choose a reason for hiding this comment

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

I assume that all the tests are green but as per my comment you added a optional parameter to the public API so I think you should add tests when you call the method w/ the componentsDir parameter

over to you @matteofigus

@@ -25,11 +25,12 @@ var registerStaticMocks = function(mocks, logger){
});
};

var registerDynamicMocks = function(mocks, logger){
var registerDynamicMocks = function(mocks, ocJsonPath, logger){
Copy link
Member

Choose a reason for hiding this comment

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

I think you should rename this parameter into ocJsonLocation as per the way you invoke the method below in line 97

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

};

module.exports = function(logger, componentsDir){
componentsDir = path.resolve(componentsDir || '.');
Copy link
Member

Choose a reason for hiding this comment

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

so if you do not pass the componentsDir parameter you set it to '. correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

correct.

describe('when oc.json is missing', function(){
var result;
beforeEach(function(){
initialise({ existsSync: sinon.stub().returns(false) });
result = getMockedPlugins(console);
result = getMockedPlugins(console, '/root/components/');
Copy link
Member

Choose a reason for hiding this comment

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

I see that now all the tests are passing the path parameter to the getMockedPlugins method; I think we should have tests w/o the second parameter as well what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can do it.

if(!content.mocks || !content.mocks.plugins){
return plugins;
}

logger.log(colors.yellow(strings.messages.cli.REGISTERING_MOCKED_PLUGINS));

plugins = plugins.concat(registerStaticMocks(content.mocks.plugins.static, logger));
plugins = plugins.concat(registerDynamicMocks(content.mocks.plugins.dynamic, logger));
plugins = plugins.concat(registerDynamicMocks(content.mocks.plugins.dynamic, ocJsonLocation, logger));
Copy link
Member

Choose a reason for hiding this comment

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

I am also thinking about preserve the same "pattern" between the methods (registerStaticMocks and registerDynamicMocks) such as adding the extra parameter either at the very beginning or after "logger" but not in between 😅

registerDynamicMocks(ocJsonLocation, content.mocks.plugins.dynamic, logger)

or

registerDynamicMocks(content.mocks.plugins.dynamic, logger, ocJsonLocation)

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@mattiaerre
Copy link
Member

LGTM 👍 (squashmerge)

@mattiaerre mattiaerre merged commit 0dafa6a into master Oct 24, 2016
@mattiaerre mattiaerre deleted the ocjson branch October 24, 2016 13:47
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