opensearch-project / reporting-cli

Apache License 2.0
4 stars 13 forks source link

Refactor cli #12

Open rupal-bq opened 1 year ago

rupal-bq commented 1 year ago
          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 https://github.com/opensearch-project/reporting-cli/pull/11#discussion_r1085898148_

rupal-bq commented 1 year ago

address following comments in refactor:

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.

- https://github.com/opensearch-project/reporting-cli/pull/40#discussion_r1160820517

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

- https://github.com/opensearch-project/reporting-cli/pull/40#discussion_r1160822411

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.

- https://github.com/opensearch-project/reporting-cli/pull/40#discussion_r1160823172

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 commented 1 year ago

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