Path mappings #175

johnrom · 2017-06-25T21:52:39Z

Added Path Mapping Support via pathMappings option. Added Tests. Reorganized test project to support path mappings.

fixes #54

…ed multiple paths to test project. Renamed to pathMappings to reflect original issue. Added unix fix

# Conflicts: # package.json # src/paths.ts

aminpaks · 2017-06-25T21:59:47Z

src/paths.ts

        serverPath = serverPath.substr(1);
    }
+
+    if ( pathMapping !== {} && typeof pathMapping !== 'undefined' ) {


Object references are never the same thus {} !== {} always is true no?

hm, you're correct.

Just check for own properties by Object.keys maybe?

removed in d005226

codecov · 2017-06-25T22:05:42Z

Codecov Report

Merging #175 into master will increase coverage by 3.47%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
+ Coverage   76.34%   79.81%   +3.47%     
==========================================
  Files           2        2              
  Lines          93      109      +16     
  Branches       17       22       +5     
==========================================
+ Hits           71       87      +16     
  Misses         14       14              
  Partials        8        8

Impacted Files	Coverage Δ
src/paths.ts	`94.11% <100%> (+2.68%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6253d4f...043adbf. Read the comment docs.

felixfbecker · 2017-06-25T22:09:34Z

src/paths.ts


 /** converts a server-side XDebug file URI to a local path for VS Code with respect to source root settings */
-export function convertDebuggerPathToClient(fileUri: string|url.Url, localSourceRoot?: string, serverSourceRoot?: string): string {
+export function convertDebuggerPathToClient(fileUri: string|url.Url, localSourceRoot?: string, serverSourceRoot?: string, pathMapping?: { [index: string]: string; }): string {


isn't localSourceRoot and serverSourceRoot obsolete with pathMappings?

Users that update will use "localSourceRoot" and "serverSourceRoot". It was easier to be backwards compatible with an optional extra argument, but I can certainly reformat it to inject localSourceRoot and serverSourceRoot into the config object if they exist.

(this is my first open source node contribution so I apologize for any code style differences)

BC is great, but this function should only take one source of truth for mappings

felixfbecker · 2017-06-25T22:09:46Z

src/paths.ts

+
    // strip the trailing slash from Windows paths (indicated by a drive letter with a colon)
    const serverIsWindows = /^\/[a-zA-Z]:\//.test(serverPath);
+


please don't try to modify the existing code style.

felixfbecker · 2017-06-25T22:10:06Z

src/paths.ts

        serverPath = serverPath.substr(1);
    }
+
+    if ( typeof pathMapping !== 'undefined' ) {


please use the existing code style (no spaces inside parens)

No linting!?

There is linting, but this isn't caught.

Maybe turn this on => Whitespace

@felixfbecker Just a friendly suggestion: tslint now has the space within parens, you may enable it.

felixfbecker · 2017-06-25T22:10:35Z

src/paths.ts

+            // normalize slashes for windows-to-unix
+            // I thought normalize did this automagically..
+            if ( process.platform !== 'win32' ) {
+                mappedServerSource = mappedServerSource.replace(/\\/g, '/');


what if the server is Windows?

This handles the case where local is Linux and server is Windows. This should be the only time backslashes need be converted because local windows will normalize the backslashes in path.relative just fine. Linux just sees the backslashes as part of the filename so path.relative returns the wrong value.

felixfbecker · 2017-06-25T22:12:15Z

src/test/paths.ts

 describe('paths', () => {
+    let undef: string;
+
+    const pathMapping = {


I prefer to define the test cases through mocha's it DSL over having a config object, it's a lot easier to debug if the values are always inside the test

felixfbecker · 2017-06-25T22:12:41Z

src/test/paths.ts

+            // unix to unix
+            it('should convert a unix path to a unix URI', () => {
+                // site
+                assert.equal( convertClientPathToDebugger(sources.unix.site, undef, undef, pathMapping.unixToUnix), results.unix.site );


why use undef and not undefined?

felixfbecker · 2017-06-25T22:14:10Z

src/test/adapter.ts

 describe('PHP Debug Adapter', () => {

-    const TEST_PROJECT = path.normalize(__dirname + '/../../testproject');
+    const TEST_PROJECT = path.normalize(__dirname + '/../../testproject/main');


why is this needed?

Some tests use /variables.php which has been moved to /testproject/main/variables.php to test multiple source roots.

but I don't see any new tests that rely on this?

I'm assuming that you're requesting a test that involves /testproject/secondary? Please give me a little direction on this.

I'm fine without a test like that given that tests/paths.ts unit-tests the path mapping, but I don't understand why all these files have to be moved then if they are essentially not used at all.

when testing development of this feature, I had to test that multiple paths were working, so I created multiple paths in testproject. I thought that in the future other developers would also need to test multiple paths, but if you'd like I can move the files back.

The testproject should only contain fixtures used by (automatic) tests

felixfbecker · 2017-06-25T22:41:58Z

src/paths.ts

+                let relative: number = localRelative.indexOf('..');
+
+                // if does not start with ..
+                if ( relative !== 0 ) {


this is not used anywhere else, so no reason to save it in a variable. You can also use .includes() if I'm not mistaken.

felixfbecker · 2017-06-25T22:42:37Z

src/paths.ts

+    if ( typeof pathMapping !== 'undefined' ) {
+        let mappedServerSource: string;
+
+        for (mappedServerSource of Object.keys(pathMapping) ) {


you should declare the let here so it's scoped to the loop body

…path conversion, reverted back to original code styles, various simplifications

johnrom · 2017-06-26T21:10:23Z

I believe I've made all requested changes. Let me know if you need anything else.

felixfbecker · 2017-06-30T09:12:02Z

src/paths.ts

-export function convertDebuggerPathToClient(fileUri: string|url.Url, localSourceRoot?: string, serverSourceRoot?: string): string {
+export function convertDebuggerPathToClient(fileUri: string|url.Url, pathMapping?: { [index: string]: string; }): string {
+    let localSourceRoot: string = '';
+    let serverSourceRoot: string = '';


why initialize with an empty string?

if these aren't initialized, then I receive an error testing for serverSourceRoot && localSourceRoot "variable is used before it is assigned"

You need to type them as string | undefined

felixfbecker · 2017-06-30T09:12:23Z

src/paths.ts

    if (serverIsWindows) {
        serverPath = serverPath.substr(1);
    }
+    if (typeof pathMapping !== 'undefined') {


shorten to if (pathMapping)

felixfbecker · 2017-06-30T09:12:53Z

src/paths.ts

        serverPath = serverPath.substr(1);
    }
+    if (typeof pathMapping !== 'undefined') {
+        for (let mappedServerSource of Object.keys(pathMapping) ) {


const, I would name this mappedServerPath

mappedServerSource (or mappedServerPath) is modified by the slash normalization below

felixfbecker · 2017-06-30T09:13:26Z

src/paths.ts

    }
+    if (typeof pathMapping !== 'undefined') {
+        for (let mappedServerSource of Object.keys(pathMapping) ) {
+            let mappedLocalSource: string = pathMapping[mappedServerSource];


don't annotate types that can be inferred

felixfbecker · 2017-06-30T09:13:39Z

src/paths.ts

+    }
    let localPath: string;
-    if (serverSourceRoot && localSourceRoot) {
+    if (serverSourceRoot !== '' && localSourceRoot !== '') {


this change is unneeded

felixfbecker · 2017-06-30T09:16:22Z

src/phpDebug.ts

    }

    protected async launchRequest(response: VSCodeDebugProtocol.LaunchResponse, args: LaunchRequestArguments) {
+        if ( args.hasOwnProperty('localSourceRoot') && typeof args.localSourceRoot !== 'undefined' && args.hasOwnProperty('serverSourceRoot') && typeof args.serverSourceRoot !== 'undefined') {


shorten to if (args.localSourceRoot && args.serverSourceRoot)

felixfbecker · 2017-06-30T09:16:37Z

src/phpDebug.ts

    protected async launchRequest(response: VSCodeDebugProtocol.LaunchResponse, args: LaunchRequestArguments) {
+        if ( args.hasOwnProperty('localSourceRoot') && typeof args.localSourceRoot !== 'undefined' && args.hasOwnProperty('serverSourceRoot') && typeof args.serverSourceRoot !== 'undefined') {
+            let pathMappings: {[index: string]: string} = {};
+            if ( args.hasOwnProperty('pathMappings') && typeof args.pathMappings !== 'undefined') {


felixfbecker · 2017-06-30T09:17:59Z

src/paths.ts

+        for (let mappedServerSource of Object.keys(pathMapping) ) {
+            let mappedLocalSource: string = pathMapping[mappedServerSource];
+            // normalize slashes for windows-to-unix
+            if (process.platform !== 'win32') {


I don't think this check is needed, this should be a pure function

felixfbecker · 2017-06-30T09:18:15Z

src/test/paths.ts

-                    'file:///C:/Program%20Files/Apache/2.4/htdocs/test.php'
-                );
+            // unix to windows
+            (process.platform === 'win32' ? it : it.skip)('should convert a unix path to a windows URI', () => {


if the platform check is removed, these should always run

felixfbecker · 2017-06-30T09:18:49Z

testproject/.vscode/launch.json

+      "externalConsole": false,
+      "pathMappings": {
+        "/var/www/html": "${workspaceRoot}"
+      }


this setting shouldn't be the default here

…, avoided win32 checks, deprecated options, fixed tests, updated docs

johnrom · 2017-07-10T20:09:04Z

FYI, I believe I've made all requested changes here.

johnrom · 2017-09-14T16:40:26Z

FYI, I believe I've made all requested changes here. Let me know if there's anything else, or at least remove the "requested changes" attribute if they have been made properly -- if that's possible.

lbod · 2017-11-21T22:22:16Z

@johnrom thanks for this. Works a treat with my first attempt at debugging in vscode with docker wordpress/xdebug. It'll be vscode itself but the debugger response into mapped paths is very fast compared to atom.

johnrom and others added 5 commits May 30, 2017 17:02

set typescript version because 2.2+ breaks things

8ba2940

Path Mappings and related tests

591a839

Correct order of conditionals

3de4c08

Flipped to Server => Local for Path Mapping. Also fixed tests and add…

d075ea4

…ed multiple paths to test project. Renamed to pathMappings to reflect original issue. Added unix fix

Merge branch 'master' into 54_PathMappings

70b1b4c

# Conflicts: # package.json # src/paths.ts

aminpaks reviewed Jun 25, 2017

View reviewed changes

fixing incorrect quotes

0df4fbe

{} !== {}

d005226

felixfbecker requested changes Jun 25, 2017

View reviewed changes

johnrom added 4 commits June 26, 2017 14:29

Rendered config to inline Mocha DSL, used single source of truth for …

4eb1b3e

…path conversion, reverted back to original code styles, various simplifications

Fixed white spaces, removed changes to testproject

5d190d8

Fixed debug adapter test

3f44427

Removed commentary

e709546

felixfbecker requested changes Jun 30, 2017

View reviewed changes

Removed types that could be inferred, switched to const when possible…

4ba7831

…, avoided win32 checks, deprecated options, fixed tests, updated docs

felixfbecker force-pushed the master branch from 108db9c to 02acaf3 Compare July 1, 2017 12:51

johnrom added 2 commits July 15, 2017 16:21

Removed string initialization, added deprecation message

987d04f

Merge remote-tracking branch 'upstream/master' into 54_PathMappings

f979c12

felixfbecker changed the title ~~54 path mappings~~ Path mappings Jul 15, 2017

Merge branch 'master' into 54_PathMappings

433126e

felixfbecker mentioned this pull request Nov 14, 2017

Mapping more than one path for source code #54

Closed

Merge branch 'master' into 54_PathMappings

1d92d26

felixfbecker approved these changes Nov 21, 2017

View reviewed changes

felixfbecker added 2 commits November 20, 2017 23:03

Merge branch 'master' into 54_PathMappings

aae2628

Merge branch 'master' into 54_PathMappings

043adbf

felixfbecker merged commit 24eafe9 into xdebug:master Nov 21, 2017

ZurabWeb mentioned this pull request Jan 2, 2018

Support Path Mappings zumba-bash-tools/dev-tools-bash#22

Merged

snyk-bot mentioned this pull request Dec 23, 2021

[Snyk] Upgrade xmldom from 0.4.0 to 0.6.0 irinazheltisheva/vscode-php-debug#3

Open


		// strip the trailing slash from Windows paths (indicated by a drive letter with a colon)
		const serverIsWindows = /^\/[a-zA-Z]:\//.test(serverPath);

Uh oh!

Path mappings #175

Path mappings #175

Uh oh!

Conversation

johnrom commented Jun 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aminpaks Jun 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 25, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aminpaks Jul 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnrom commented Jun 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnrom commented Jun 25, 2017 •

edited

Loading

aminpaks Jun 25, 2017 •

edited

Loading

codecov bot commented Jun 25, 2017 •

edited by ghost

Loading

aminpaks Jul 22, 2017 •

edited

Loading