Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Fix internal package import #1535

Merged
merged 3 commits into from
Feb 28, 2018
Merged

Fix internal package import #1535

merged 3 commits into from
Feb 28, 2018

Conversation

uudashr
Copy link
Contributor

@uudashr uudashr commented Feb 23, 2018

Fixes for case:
package to import: github.com/uudashr/myproject/internal/config'
importer package: /Users/uudashr/go/src/github.com/uudashr/myproject-monitoring/cmd/app-monitor

Expect: Shouldn't allowed

@uudashr
Copy link
Contributor Author

uudashr commented Feb 23, 2018

@ramya-rao-a please check

@uudashr
Copy link
Contributor Author

uudashr commented Feb 23, 2018

@ramya-rao-a maybe it's a good idea to add unit tests for this project, test that doesn't rely to UI, and relatively fast to run.
I think we have lot codes can be test without requiring UI framework, this will become big saviour.

Haven't figure out how to do it in this project, the idea to separate the test command.
Ex:

  • use variable
    • unit test SCOPE=unit npm test
    • ui test SCOPE=ui npm test
  • dedicated script npm test-unit

not sure which follows best practice

@@ -238,7 +238,7 @@ export function isAllowToImportPackage(toDirPath: string, currentWorkspace: stri
let internalPkgFound = pkgPath.match(/\/internal\/|\/internal$/);
if (internalPkgFound) {
let rootProjectForInternalPkg = path.join(currentWorkspace, pkgPath.substr(0, internalPkgFound.index));
return toDirPath.startsWith(rootProjectForInternalPkg);
return toDirPath.startsWith(rootProjectForInternalPkg + '/') || toDirPath === rootProjectForInternalPkg;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance in Windows this would be \ instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure... it based on pkg. It will fail on let internalPkgFound = pkgPath.match(/\/internal\/|\/internal$/); also..
Need Windows machine

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 see.. will push latest changes

@ramya-rao-a
Copy link
Contributor

Refactoring this project for tests has been one thing that I wanted to do for a long time. Having tests that do not rely on the extension host spinning up a new window will definitely help. I am not sure how to go about that at the moment, but I'll give that a try soon

@ramya-rao-a ramya-rao-a merged commit 41c5b3b into microsoft:master Feb 28, 2018
@uudashr
Copy link
Contributor Author

uudashr commented Mar 1, 2018

Awesome 👍

@ramya-rao-a
Copy link
Contributor

This fix is now available in the latest update (0.6.78) to the Go extension

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

Successfully merging this pull request may close these issues.

2 participants