Skip to content
This repository has been archived by the owner on Aug 27, 2019. It is now read-only.

Updates to documentation #2

Merged
merged 12 commits into from
Feb 3, 2016
Merged

Updates to documentation #2

merged 12 commits into from
Feb 3, 2016

Conversation

IanLee1521
Copy link
Member

This is to help work on #1. I thought I would start pushing my edits and updates so that I could start getting feedback. I feel as though this could be ready for merging now, but that really I'd like to add some more / make a few more changes.

Couple of major updates / changes.

  • I added a setup.py file to allow packaging up this tool and distributing it.
  • I created a package on pypi to allow this to simply be pip install slack-emoji-search capable. @micahsaul , I am of course happy to at least add, or even transfer the package to you if you prefer.

I feel like the need to create an api_token.py file (currently what is documented) is a bit clunky... I propose that this might want to change to one of two approaches:

Added a --api-token=ABCXYZ commandline argument to allow it to be passed in to the script. And / or add the ability for the scrip to look in a particular location (something like ~/.slack/api_token ?) that could be where users are then instructed to look for it / to store their key. Thoughts?

As a side note -- Unfortunately, I don't have a team on Slack, so I can't get an API token, and therefore can't fully test things out for you, but just doing source code reading so far, looks pretty good.

* Added missing reference to virtualenvwwrapper which is actually where
  the `mkvirtualenv` and `workon` commands come from
* Include links to installation instructions for pip, virtualenv, and
  virtualenvwrapper
* Added steps for installing pip, virtualenv, and virtualenvwrapper
  using `ensurepip` in Python 2.7.9+ and Python 3.4+
version='0.0.1',
description='Search across Slack for messages with a specific emoji.',
author='Micah Saul',
author_email='micah.sual@gsa.gov',
Copy link
Member

Choose a reason for hiding this comment

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

Should be Micah.Saul@gsa.gov

@IanLee1521
Copy link
Member Author

@wslack fixed the author_email typo, thanks for spotting that!


Ensure Python and virtualenv are set up. If you've run the [laptop script](https://github.com/18F/laptop), you're all good.
The simplest way to install this package is to use [pip](https://pip.pypa.io/en/stable/installing/) to install the package from the [Python Package Index](https://https://pypi.python.org/pypi/slack-emoji-search):
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 868330a, thanks!

@brittag
Copy link
Contributor

brittag commented Feb 3, 2016

Thanks @IanLee1521, setting up a pypi package is a nice idea - this seems like a helpful improvement overall! And thanks for taking a look through the code. Could it make sense to update the description at https://pypi.python.org/pypi/slack-emoji-search to ask people to refer to this readme, so that the instructions don't get out of sync when people update this one? (I don't know if that's a reasonable practice on pypi.)

@IanLee1521
Copy link
Member Author

@brittag -- Thanks for the feedback. I updated the description on pypi on a one off basis. I have to double check how to do it dynamically.

For what it's worth, the default mode for PyPI is to use the README as the description, snapshotted at that version that is uploaded (so theoretically the Git repo might be newer than the description on PyPI, but that ties the description on PyPI to that version that it is holding. Does that make sense?

@micahsaul
Copy link
Contributor

Looks good to me! Thanks @IanLee1521! I opened #3 to implement your proposal for better token handling.

micahsaul added a commit that referenced this pull request Feb 3, 2016
@micahsaul micahsaul merged commit 9beb114 into 18F:master Feb 3, 2016
@brittag
Copy link
Contributor

brittag commented Feb 3, 2016

Thanks again @IanLee1521! That makes sense. It could be helpful for us to build a habit of making PyPI packages for utilities; we'll think about how to make that happen.

@IanLee1521
Copy link
Member Author

@micahsaul see #5 for a possible crack at what I proposed (just adding the CLI flag as an alternative to creating the api_token.py file.

@brittag -- Happy to help! Let me know if you need / want any other input on that. I'm still getting up to speed a bit, but happy to help out (also I'm just very pro the-work-that-18F-is-doing, especially when I put on one of my work hats.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants