Skip to content

add login into command list & alphabetize #713

Merged
northyg merged 3 commits into
mainfrom
giselle/src-add-login-to-list
Mar 21, 2022
Merged

add login into command list & alphabetize #713
northyg merged 3 commits into
mainfrom
giselle/src-add-login-to-list

Conversation

@northyg

@northyg northyg commented Mar 17, 2022

Copy link
Copy Markdown
Contributor

Fixes https://github.com/sourcegraph/sourcegraph/issues/32450 and adds a bit more!

Test plan

  • Added login command to the list
  • Noticed list was not alphabetized so went a step further and re-ordered the commands
  • Confirmed changes in src-cli dev environment look visually correct (use spaces versus tabs 🙃 )

image

@northyg northyg requested a review from malomarrec March 17, 2022 22:53
Comment thread cmd/src/main.go Outdated
extensions,ext manages extensions (experimental)
batch manages batch changes
lsif manages LSIF data
login authenticate to a Sourcegraph instance with your user credentials

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Although this looks visually aligned on an 8 space tab width, the other lines actually use spaces rather than tabs between the command name and the description (ie between login and authenticate in this case). Would you be able to change this to match the other lines with spaces, please? (Alternatively, I'm happy to do it and push it to your branch, if you'd prefer!)

@northyg northyg Mar 18, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hello @LawnGnome 👋 Thanks for having a look! Hmm, I added the line with spaces and double checked the file in VSC just now. It looks like there are spaces versus tabs, but perhaps I'm missing something? 🤔

image

If I did, could you let me know where the issue is and I am happy to fix it 😸

@abeatrix abeatrix Mar 18, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@northyg This is what I see when I check out your branch:
image
It looks like it's still using 2 tabs instead of 11 spaces, and the list is not alphabetized as shown in your picture 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@LawnGnome Aha! @abeatrix and I figured out the problem. I got turned around in the other branch I had made, and didn't push the changes correctly. 🐝 Helped me sort it out. I also made an addition to the Changelog. Can you have a look and see if its good to go ?

@malomarrec

malomarrec commented Mar 18, 2022

Copy link
Copy Markdown

Thanks for doing this! (Deferring the review to Adam)

@malomarrec malomarrec removed their request for review March 18, 2022 10:10

@abeatrix abeatrix left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM, great job! Let me know if you have questions regarding the formatting issue Adam has mentioned above, happy to help!

@northyg

northyg commented Mar 18, 2022

Copy link
Copy Markdown
Contributor Author

Thanks @abeatrix 😄 ! I re-checked the file and replied above. 🤔
Also, realized I forgot to update the CHANGELOG.md. Do you and @LawnGnome think this PR is worth adding to it? If so I can make that change as well.

@abeatrix

Copy link
Copy Markdown

Hey @northyg I just replied above. I'm wondering if we are seeing the unchanged file because you haven't pushed your local changes into the branch?

@LawnGnome LawnGnome left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great, thanks!

@northyg northyg merged commit c750d8f into main Mar 21, 2022
@northyg northyg deleted the giselle/src-add-login-to-list branch March 21, 2022 07:11
scjohns pushed a commit that referenced this pull request Apr 24, 2023
* add login into command list
* correctly push file changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add src login to src-cli help

4 participants