Fixing bug #2607 related to ModuleSpec syntax in RequiredModules #3594
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chunqingchen
suggested changes
Apr 21, 2017
Contributor
There was a problem hiding this comment.
does requiredModuleSpecification only needs to be added when currentModule is not null?
this is confusing here. if so, please add comment explains why
Author
There was a problem hiding this comment.
Originally I was thinking that both addition should be bundled together; but thinking more about it - it is better to move the new addition to its own 'if' check. Updated.
Contributor
There was a problem hiding this comment.
please also check that the module is actually loaded.
anmenaga
force-pushed
the
FixedModuleSpecSyntaxInRequiredModules
branch
from
5fc877d to
bad1221
Compare
chunqingchen
approved these changes
Apr 28, 2017
Contributor
|
good to sign off |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes issue #2607.
'RequiredModules' is a field in module manifest that can reference other modules using ModuleSpecification format.
The basic version of this format (just module name) was working fine, however there was a problem when more detailed version of the format was used (the one that uses module versions or/and GUIDs).
During module import, there is a check for cyclic references through 'RequiredModules' field. The bug was in this check for cyclic references , related to comparison rules for ModuleSpecification objects - as a result code was incorrectly reporting 'cyclic reference' error in cases when there was none.
Added tests for different ModuleSpecification formats and a test for error when there is actually a cyclic reference.
Test results before the fix:
Test results after the fix:
