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

Fix recent breaking changes to normalizePath in filebrowser #8383

Conversation

telamonian
Copy link
Member

@telamonian telamonian commented May 7, 2020

References

the last time this happened:
#6841
#6898
#6900
#6912

fixes #8382
46ec049

Code changes

reverts recent breaking changes to normalizePath (see #8382)

User-facing changes

unbreaks filebrowser. You can check out the binder build of any of our other recent PRs if you want to see the broken filebrowser in action, eg #8381:

Binder

Backwards-incompatible changes

NA

@jupyterlab-dev-mode
Copy link

jupyterlab-dev-mode bot commented May 7, 2020

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@telamonian
Copy link
Member Author

telamonian commented May 7, 2020

Interestingly, the fix in this PR breaks the current filebrowser CI test

@telamonian
Copy link
Member Author

telamonian commented May 7, 2020

I figured out the problem with the tests and fixed it. To fix it, I always pass in "/" as the first arg to resolve in normalizePath. This effectively short circuits the expansion behavior of resolve, and ensures consistent behavior in both the node and browser environments. resolve will also ensure that there is always exactly one leading slash on any path it returns, which we then strip off as per the already existing logic.

@telamonian
Copy link
Member Author

telamonian commented May 7, 2020

I added a test to filebrowser that covers cd-ing to a nested directory.

I also wanted to refactor normalizePath and add some unit tests for it, but I quickly ran into trouble. The current implementation of normalizePath is very tangled up in ContentsManager and elsewhere. It would be nice if we could figure out the absolute minimal set of behaviors required for normalizePath and then write a simplified version of it.

In retrospect, I guess this is what you were trying to do in the first place @blink1073?

@telamonian
Copy link
Member Author

telamonian commented May 7, 2020

After realizing how complete ContentsManagerMock is, I went ahead and added some unittests for normalizePath. Two caveats:

  • the "should not prepend root path to an absolute path" behavior of normalizePath seems strange and unnecessary. It would make more sense for us to either raise an error when the path arg is absolute, or to aggressively strip any leading / from path

  • in order to be able to call normalizePath in the tests, I had to move it from a Private namespace into the public API. I think it's worth it so we can have some actual unittests to help us flesh out and robustify an otherwise fragile function

@blink1073
Copy link
Member

blink1073 commented May 7, 2020

I agree with raising an error when the path arg is absolute. Otherwise, this looks great, thanks!

@telamonian telamonian force-pushed the fix-normalizepath-revert-modernize-tests branch from 0a7d336 to e10bef3 Compare May 12, 2020
@telamonian
Copy link
Member Author

telamonian commented May 12, 2020

normalizePath didn't make a ton of sense as part of the public API of FileBrowserModel, so I refactored it into Contents.IManager.resolvePath. The behavior of resolvePath is consistent with posix-path.resolve, but its signature is slightly different, so I think the name makes sense. I think it also makes sense to group it with the other drive-aware path handling functions in Contents.IManager (driveName, localPath, etc).

raising an error when the path arg is absolute

As for this, turns out we rely 100% of the time on the current absolute path handling

private _handleOpen(item: Contents.IModel): void {
this._onItemOpened.emit(item);
if (item.type === 'directory') {
const localPath = this._manager.services.contents.localPath(item.path);
this._model
.cd(`/${localPath}`)
.catch(error => showErrorMessage('Open directory', error));
} else {
const path = item.path;
this._manager.openOrReveal(path);
}
}

so for right now, we should leave the relative/absolute path handling as-is here.

See #6841, #6900, #6912 for info on why we handle paths this way

@telamonian telamonian requested review from blink1073 and afshin May 12, 2020
Copy link
Member

@blink1073 blink1073 left a comment

Thanks! CI Failures are unrelated, but should be addressed before we merge this.

@telamonian
Copy link
Member Author

telamonian commented May 13, 2020

yeah, the usage tests keep timing out, especially the python 3.5 version for some reason

@blink1073
Copy link
Member

blink1073 commented May 13, 2020

I think it is related to jupyterlab/jupyterlab_server#88

@blink1073
Copy link
Member

blink1073 commented May 16, 2020

I'm addressing CI failures in #8433. Merging this now.

@blink1073 blink1073 merged commit fd2d816 into jupyterlab:master May 16, 2020
38 of 40 checks passed
@blink1073 blink1073 added this to the 2.2 milestone May 16, 2020
@github-actions github-actions bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Feb 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pkg:filebrowser pkg:services status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants