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

Some ideas #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Some ideas #9

wants to merge 1 commit into from

Conversation

gicta
Copy link

@gicta gicta commented Feb 6, 2022

Hello,
Not sure this fits your product vision (which is quite elegant), but I wanted the option to:

  1. Store key files outside the main project path: I added a -k or --key-path arg that takes a path
  2. Use a non-default text editor (although the code that detects the default editor is impressive): I added a -e or --editor arg that overrides the default editor, and if it is "code", I make sure to run "code --wait" so that the UI doesn't return to the spawning window until after the file is closed.

Windows 10:
Tested new & edit with no arguments, tested edit with -k, tested edit with -e, tested edit with -k and -e - I think it works.
I don't have a linux distro handy. Yes, I know that is sad.

All the best either way and thanks for this package :-)

…or to a non-default editor, and ensuring code runs as code --wait so changes are captured properly when using vscode
@Pik-9
Copy link
Owner

Pik-9 commented Feb 8, 2022

Hello @gicta,

thank you for your contribution. Looking through your suggestions and code I came up with a few issues to consider:

Commandline arguments vs environment variables

You added the two options as command line arguments to the executable script. But the important part of this package is the part you import into another node application, that will not have those command line arguments.
This is why I opted for environment variables as a way of passing arguments to the app instead.

In fact the environment variable to choose the text editor already exists:

EDITOR=my-fancy-editor npx schluessel

But checking for a specific environment variable for an arbitrary key file path is a good suggestion. 😉

The problem with a setKeyPath() function

You added a setKeyPath() function that sets the key path to look for the key file.
Unfortunately this approach is not gonna work, since the key file is searched for and loaded only once at the very beginning, right away with the import. This means calling this function afterwards will not change anything.
Again: The only viable way I see here is an environment variable that is accessible during this import process.

Coding style

I know this sounds a bit pettifogging, but I assure you my intentions are purely constructive.
This project's source code strictly complies to the Airbnb coding style.

There is a linter eslint in place to make sure that the given code complies to it. You can use it to see violations and get hints on how to fix them:

npm run lint

TL;DR

In short the option to use a custom text editor already exists through the EDITOR environment variable.
I don't see the benefits of passing these options via command line arguments since they will be unavailable in projects importing schluessel.
I do see however the benefits of an option to supply a non standard key file path, but again: an environment variable would be the better approach here in my opinion.

Kind regards,
🂩

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.

2 participants