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

Feature/cli #54

Merged
merged 4 commits into from
Oct 30, 2022
Merged

Feature/cli #54

merged 4 commits into from
Oct 30, 2022

Conversation

kajahno
Copy link
Owner

@kajahno kajahno commented Oct 29, 2022

There's a few race conditions, but I've done the minimum required for everything function.

This is how the interface looks:

npm run start -- stats-daily --help

> elprimobot@1.0.0 start
> node app.js "stats-daily" "--help"

discord client is ready
elprimobot stats-daily

Sends Discord users daily chat activity stats

Options:
      --help     Show help                                             [boolean]
      --version  Show version number                                   [boolean]
  -v, --verbose  Run with verbose logging                              [boolean]

I've tested everything. All working

* separate actions into commands
* leave the daemon as as another command
@kajahno kajahno requested a review from vetom October 29, 2022 11:34
@kajahno kajahno linked an issue Oct 29, 2022 that may be closed by this pull request
@kajahno kajahno requested a review from nramirez October 29, 2022 11:35
app.js Show resolved Hide resolved
app.js Show resolved Hide resolved
utils.js Outdated Show resolved Hide resolved
@nramirez
Copy link
Collaborator

nramirez commented Oct 30, 2022

out of curiosity, what are some of the race conditions?

@kajahno
Copy link
Owner Author

kajahno commented Oct 30, 2022

@nramirez I've created the following issue for the refactor #58.

I think it's going to require changing quite a few things for this, for example, to pass the discord client around.

One of the race conditions is that sometimes when the client initializes and we immediately call the discord APIs that have the .channel, it wouldn't grab anything and would raise an exception instead.

@nramirez
Copy link
Collaborator

That race condition won't happen if you call the client like this => 996ea55#diff-fda8f0e13abe3d5c5685ad895fa528dd6067c41b66b6ebc6d1de71ca9a704d8dR55

I agree you can refactor later, but given that my changes now landed you can rebase and reuse the new builder.

Copy link
Collaborator

@nramirez nramirez left a comment

Choose a reason for hiding this comment

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

LGTM, sure thing we can improve this in the next follow-ups.
I would recommend rebasing to avoid the dup with the client creation.

@vetom vetom mentioned this pull request Oct 30, 2022
Copy link
Collaborator

@vetom vetom left a comment

Choose a reason for hiding this comment

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

Agree with Naz's comments, well done!

package.json Show resolved Hide resolved
@kajahno kajahno merged commit c3c9498 into main Oct 30, 2022
@kajahno kajahno deleted the feature/cli branch October 30, 2022 21:27
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.

Create CLI for the bot
3 participants