-
Notifications
You must be signed in to change notification settings - Fork 318
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
Unable to set email/username on a remote server #1324
Conversation
Thanks for submitting your first pull request! You are awesome! 🤗 |
src/components/GitPanel.tsx
Outdated
@@ -842,7 +842,7 @@ export class GitPanel extends React.Component<IGitPanelProps, IGitPanelState> { | |||
* @param path - repository path | |||
*/ | |||
private async _hasIdentity(path: string | null): Promise<string | null> { | |||
if (!path) { | |||
if (path == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The liner suggests to use triple equals here. The argument annotation says it cannot be undefined so it should be equivalent:
if (path == null) { | |
if (path === null) { |
Unless this fix depends on it, then we need to change linter eqeqeq rule to smart and add undefined in the argument.
Also, can you add a test?
Nice! Thanks @jzhang20133! This LGTM. @fcollonval I don't have access on this repo. Would you mind helping me merge and release a patch here? I can pick this up too if you want to give me access here. |
66d77a4
to
a22a89c
Compare
@krassowski I dove into this a bit. It appears that we're skipping most unit tests for this panel since the JupyterLab 4 release. (see here, here, here, here, and here). I believe this is the reason the current bug wasn't detected in the first place. Removing and of the I think it's beyond the scope of this current PR to fix all of these failing unit tests. For the sake of addressing the current bug, I would propose that we merge the current PR without additional unit testing. Further, it seems that the workflows for this repo have been failing for quite awhile due to some dependency resolution errors. |
I've opened the following issue to track work on re-enabling the skipped unit tests (hopefully in a follow-up PR): #1325 |
I've added you as a collaborator, but let's get the CI green before merging - thank you for opening #1326! |
a22a89c
to
ab47c21
Compare
Done, @krassowski 🚀 I'll try to get the (skipped) unit tests working for the Git Panel. If I can't get that done today, I don't think we should block this PR. |
ab47c21
to
b8dfe80
Compare
309235b
to
8c87bfb
Compare
ad9ef9d
to
eebda6b
Compare
d93d046
to
4bf605b
Compare
4bf605b
to
b27f819
Compare
Amazing work, @jzhang20133! Thank you so much for taking it the extra mile to fix all unit tests and adding a few new ones for this PR! 🚀 |
We'll need help with releasing this bug fix, since we don't have admin access to this repo. I think it would be worth updating the Github workflows for releasing to use the latest work happening in Jupyter-releaser with project level PyPI tokens. |
Thanks a lot @jzhang20133 @Zsailer and @krassowski for the latest work; v0.50.1 has been released with the latest fixes. |
Thank you, @fcollonval! We appreciate you and @krassowski tremendously! ❤️ |
Fixes #1323