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

Unable to set email/username on a remote server #1324

Merged
merged 1 commit into from
May 16, 2024

Conversation

jzhang20133
Copy link

Fixes #1323

Copy link

welcome bot commented May 14, 2024

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

Copy link

Binder 👈 Launch a Binder on branch jzhang20133/jupyterlab-git/email_username_popup

@@ -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) {
Copy link
Member

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:

Suggested change
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?

@Zsailer
Copy link
Member

Zsailer commented May 14, 2024

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.

@Zsailer
Copy link
Member

Zsailer commented May 14, 2024

Also, can you add a test?

@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 .skip calls causes these tests to fail, so it seems that the API changed somewhere along the way and the tests were never updated?

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.

@Zsailer
Copy link
Member

Zsailer commented May 14, 2024

I've opened the following issue to track work on re-enabling the skipped unit tests (hopefully in a follow-up PR): #1325

@krassowski
Copy link
Member

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.

I've added you as a collaborator, but let's get the CI green before merging - thank you for opening #1326!

@Zsailer
Copy link
Member

Zsailer commented May 15, 2024

but let's get the CI green before merging

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.

@Zsailer Zsailer mentioned this pull request May 15, 2024
@Zsailer Zsailer force-pushed the email_username_popup branch 2 times, most recently from ad9ef9d to eebda6b Compare May 15, 2024 21:24
@jzhang20133 jzhang20133 force-pushed the email_username_popup branch 4 times, most recently from d93d046 to 4bf605b Compare May 15, 2024 23:07
@Zsailer
Copy link
Member

Zsailer commented May 16, 2024

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! 🚀

@Zsailer Zsailer merged commit d0480eb into jupyterlab:main May 16, 2024
8 checks passed
Copy link

welcome bot commented May 16, 2024

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@Zsailer
Copy link
Member

Zsailer commented May 16, 2024

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.

@fcollonval
Copy link
Member

Thanks a lot @jzhang20133 @Zsailer and @krassowski for the latest work; v0.50.1 has been released with the latest fixes.

@Zsailer
Copy link
Member

Zsailer commented May 16, 2024

Thank you, @fcollonval! We appreciate you and @krassowski tremendously! ❤️

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

Successfully merging this pull request may close these issues.

Unable to set user name and email
4 participants