Clarifying readme for auth keys - #288
Conversation
greglucas
left a comment
greglucas
left a comment
There was a problem hiding this comment.
I also missed the $ additions in the previous PR. I don't love those because then I can't copy-paste. Because it is in a code block I don't think we need the $ my-command prefix. But there is apparently disagreement on preferences here too :) https://github.com/orgs/community/discussions/35615
All in all, this is an improvement and looks good to me! Feel free to take or leave the suggestions at your own will, I'm fine with any of it.
|
|
||
| ```bash | ||
| $ IMAP_API_KEY=<your-api-key> | ||
| $ export IMAP_API_KEY=<your-api-key> |
There was a problem hiding this comment.
I still have a slight preference for doing this on the same line like we had it before instead of an export (which is also shell specific).
#286 (comment)
But, I agree this does need to change.
There was a problem hiding this comment.
Would it be enough to swap the lines, so the CLI flag is first? I'm not sure I understand what you mean about doing it on the same line... I can also remove this code block entirely since your point about the export is also valid.
If you think we should not set a key for an entire session, that's definitely valid, but that should be changed in the code rather than having it be undocumented behavior. I prefer to set a key for the entire session but I am very careful about it lol
There was a problem hiding this comment.
Sorry, what we had before was:
IMAP_API_KEY=<your-api-key> imap-data-access ...
Which only sets the API Key for that one command and not the entire session, so makes you think about exporting it rather than defaulting to the export. That is all I was trying to say to squash these two lines together. Not change any behavior.
There was a problem hiding this comment.
Ah fab, yeah I can change that!
fc4814f
into
IMAP-Science-Operations-Center:main
2 participants
Change Summary
Updated readme to clarify some confusion I had