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

Add option to fetch login or email #216

Merged
merged 3 commits into from
Mar 8, 2024
Merged

Conversation

mzellho
Copy link
Contributor

@mzellho mzellho commented Mar 7, 2024

This PR adds a little option to lookup the login for a credential, similar to the password-option.

@Mikescops
Copy link
Member

Hello,

Thanks for taking the time to open a PR here. I like the idea but I think dcli login is a little bit confusing as it may let think that's the way to login into the Dashlane account.
Also, to avoid duplicating much code I think it would be better to use an option on dcli password instead. That way we can reuse most of the code and just make a condition when doing the copy in the pastebin. Something like dcli password --login.

Let me know what you think about this.

@Mikescops Mikescops self-assigned this Mar 7, 2024
@Mikescops Mikescops added the features New feature or request label Mar 7, 2024
@Mikescops Mikescops assigned mzellho and unassigned Mikescops Mar 7, 2024
@mzellho
Copy link
Contributor Author

mzellho commented Mar 7, 2024

My pleasure, thanks so much for this awesome tool that saves me so much time every day.

Initially, I was going with "username", but then realised that what I wanted to fetch is the login. So if you are okay with "user" we could also use this?

Of course, sub-options of password would also be possible and produce less duplicate code.

Your call, really 🙂

@Mikescops
Copy link
Member

Alright, I'd go for the sub-option then, when I initially made the different commands I followed the content-types that we have in the vault : passwords, secure notes, secrets.

@mzellho mzellho force-pushed the master branch 2 times, most recently from de7d6b9 to a8b5894 Compare March 7, 2024 13:09
@mzellho mzellho removed their assignment Mar 7, 2024
@mzellho mzellho changed the title Add option to fetch login Add option to fetch login or email Mar 7, 2024
@mzellho
Copy link
Contributor Author

mzellho commented Mar 7, 2024

Alright, I added the sub-options discussed and also introduced an option to to fetch the email. Any other bits of the credentials worth fetching via the cli?

src/commands/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@Mikescops Mikescops left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a naming recommendation and we can go with this, thanks!

@mzellho
Copy link
Contributor Author

mzellho commented Mar 8, 2024

@Mikescops done, thank you!

Copy link
Member

@Mikescops Mikescops left a comment

Choose a reason for hiding this comment

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

Tested locally and it works well.

@Mikescops Mikescops merged commit b3c842f into Dashlane:master Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
features New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants