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

Add private groups to channels view and throw fatal errors #35

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

omnibrian
Copy link

@omnibrian omnibrian commented Feb 20, 2017

Hey,

Since most of my use of slack is in private channels, I decided to add a feature for it. I've implemented it by changing the slackClient so I could keep the amount of files changed to a minimum. I also realise that checking for an error to see if it's a group isn't the best of solutions but it's the only one I could think of.

I've also added a second commit where I changed the error logging from logging to a file to throwing the error message. If you don't agree with that I can change to PR to just be the first commit.

Tell me what you think.

EDIT: After noticing that the slack api doesn't react well with multi-user direct message channel names I removed them with the same filter for removing archived channels.

Thanks, Brian

@jaythomas
Copy link

Did you intend to use the Error constructor?

throw new Error('SLACK_TOKEN undefined. Please add SLACK_TOKEN to the environment variables.')

@omnibrian
Copy link
Author

Yes, I can change that to more appropriate error type if there is one, I just figured that throwing an error is cleaner than console logging an error message and exiting with a failed status.

@jaythomas
Copy link

I think throwing an error is best practice, I just meant that you are throwing a string instead of an Error object in your code the way you have it written now. It is generally best practice to wrap the string in new Error("my string") so that try/catch blocks can more easily handle the errors.

@omnibrian
Copy link
Author

Ah, my bad, I'm still learning js. I've updated the PR to use the Error constructor as you suggested, thanks!

@loflina
Copy link

loflina commented Feb 14, 2018

Thank you for this amazing tool! I love it! I'm sorry if this isn't the appropriate place for this but, can I ask why we don't show multi-user direct messages? This would be really helpful for me.

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.

3 participants