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

resolve module dependencies #106

Merged
merged 2 commits into from
May 23, 2017
Merged

resolve module dependencies #106

merged 2 commits into from
May 23, 2017

Conversation

Matt-Esch
Copy link
Contributor

Populates the resolved dependencies for each module instance in the module system. This will be used by the dependency struct generation code.

@coveralls
Copy link

coveralls commented May 23, 2017

Coverage Status

Coverage increased (+0.04%) to 73.829% when pulling 15a739c on me.resolve.deps into ab97b38 on master.

Raynos
Raynos previously requested changes May 23, 2017
Copy link
Contributor

@Raynos Raynos left a comment

Choose a reason for hiding this comment

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

Looks like merge-conflict, can get rid of duplicate loop.

Other then that, lgtm.

@@ -269,6 +269,54 @@ func (system *ModuleSystem) ResolveModules(
append(resolvedDependencies, dependencyInstance)
}
}

// Resolve the class dependencies
for _, classInstance := range classInstances {
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like merge-conflict. we have this loop twice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we have this loop twice because the current class instances need to be fully resolved before you can resolve the dependencies (because clients can have peer dependencies, for example). So basically you have to create all possible dependency ModuleInstance objects before you can populate the ResolvedDependencies. I don't think this is incorrect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you leave a comment, the loops looked identical at a glance.

@@ -169,3 +256,185 @@ func getTestDirName() string {
// If dirname is not absolute then its a package name...
return filepath.Join(os.Getenv("GOPATH"), "src", dirname)
}

func compareInstances(
Copy link
Contributor

Choose a reason for hiding this comment

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

this method can be replaced with assert.Equal(t, actual, expected)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not fully equal because there is binary content, I'm asserting a subset


if !ok {
return nil, errors.Errorf(
"Invalid class name \"%q\" in dependencies for %s %s",
Copy link
Contributor

Choose a reason for hiding this comment

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

I am guessing "Invalid class name %q in dependencies for %q %q" is what you want here?


if dependencyInstance == nil {
return nil, errors.Errorf(
"Unknown %q class depdendency \"%q\""+
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, redundant quotes for %q

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely sure that %q is right at all, but I will try to make it consistent at least.

@Matt-Esch Matt-Esch dismissed Raynos’s stale review May 23, 2017 21:16

Comments addressed inline.

@coveralls
Copy link

coveralls commented May 23, 2017

Coverage Status

Coverage increased (+0.04%) to 73.829% when pulling 028aaa4 on me.resolve.deps into ab97b38 on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants