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

Refactor cli #12

Open
rupal-bq opened this issue Jan 24, 2023 · 2 comments
Open

Refactor cli #12

rupal-bq opened this issue Jan 24, 2023 · 2 comments
Labels
enhancement New feature or request

Comments

@rupal-bq
Copy link
Contributor

          Minor: I feel the param list is a little bit long, we may consider to refactor it a little bit later.

Originally posted by @mengweieric in #11 (comment)

@rupal-bq rupal-bq changed the title Refactor Refactor cli Jan 24, 2023
@rupal-bq rupal-bq added enhancement New feature or request and removed untriaged labels Feb 9, 2023
@rupal-bq
Copy link
Contributor Author

rupal-bq commented Apr 7, 2023

address following comments in refactor:

Could you try this merge strategy :

const defaults = {
  auth: DEFAULT_AUTH,
  tenant: DEFAULT_TENANT,
  ... (etc)
}

Object.assign(defaults, ...event)
I think would both clarify the defaults, and make the fallback assignments a one-liner.
Might start to make sense to turn this into an object-reference, with an interface. Naming might be hard, though I could make more sense and less brain-power out of arguments named {emailContent, smtpConfig} with associated interfaces.

This would also mean that further config changes (new options) do not cause change on this method (or at least this method signature
This feels like making this module to two different things : 1. format/send email. 2. control UI/UX.

I think the only question in splitting this up would be if there is expected some "on-the-fly" UX while the email-sending process is ongoing... In our modern computing environment, there shouldn't be a timing issue... emails never take > 10sec so send.. or else they fail immediately.
This might be a lot more clear using a try..catch.. .and if necessary a throw on self-detected error (err value is not null?)

@rupal-bq
Copy link
Contributor Author

this works but seems backwards, it's easy to miss since the code is checking process.env[ENV_VAR.EMAIL_NOTE] in both getOptions and getEventArguments. e.g. if a config file is introduced later, this needs to also check && config_file.email_note === undefined

i think peter brought this up before, it might be better to have a default options object in getOptions. If there are any explicitly defined configs, overwrite the value with the highest priority config (command arguments > environment variables), or overwrite multiple times starting from lowest priority

@rupal-bq rupal-bq added the v1.1.0 label May 2, 2023
@rupal-bq rupal-bq removed the v1.1.0 label Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant