Skip to content
This repository has been archived by the owner on Apr 29, 2024. It is now read-only.

Approve this gem to be made public #1

Closed
7 tasks done
JacobEvelyn opened this issue Mar 17, 2017 · 5 comments
Closed
7 tasks done

Approve this gem to be made public #1

JacobEvelyn opened this issue Mar 17, 2017 · 5 comments
Assignees

Comments

@JacobEvelyn
Copy link
Member

JacobEvelyn commented Mar 17, 2017

Reviewer, please check that (this is from our usual OSS checklist here):

  • All code is as general as possible
  • Code has as few dependencies as possible
  • Gemspec has overcommit and rubocop (and any other needed linters) as development dependencies
  • License is attributed to Panorama Education
  • Tests are reasonable (I don't think this is necessary for this repo.)
  • All code is commented with YARD style (I don't think this is necessary for this repo.)
  • This gem is something that should be public (is not a security liability, contains no Panorama-specific code, does not make Panorama look bad, etc.). In particular check that I didn't accidentally commit any Panorama-specific emails or credentials (e.g. SendGrid, Google Calendar), either in this or in the reflog.

Note: I have confirmed that this code works when following the installation instructions in the README and running the code as a gem built from this code.

Context: We're open-sourcing this because several people have heard about this via word-of-mouth and asked us if they can use it. For the time being, we won't advertise this or blog about it until it has a few more users to iron out the kinks.

@forgottentea-xx
Copy link

forgottentea-xx commented Mar 21, 2017

  • In the readme, there is a reference to order_as_specified that should be changed
  • It looks like the username for the google credentials is hardcoded as CREDENTIALS_USER_ID = NORA .. should that be configurable? Its also possible I don't fully understand how that piece of the code works
  • Same thing for Pony options, specifically the domain is hardcoded to heroku.com
  • I wonder if the pony config should be loaded from its own file?

@JacobEvelyn
Copy link
Member Author

Thanks for the comments! I just pushed another commit addressing these.

Re: the credentials and application name, I think you're right that they should be configurable. Since it's been a while since I've set up the Google OAuth stuff, do you have feedback for what should go in the README here to make setup clearer?

I think for now I lean towards keeping Pony config all in the one file. It's definitely bloating a bit with that and Google Calendar stuff but since as far as I'm aware we'll likely only have one non-Panorama user for the near future (who I'll work with to make sure everything gets set up okay) I don't think optimizing the config file is worthwhile right now.

Thoughts?

@forgottentea-xx
Copy link

I think the README is clear enough, re: OAuth. A link to the documentation could be useful, but I imagine the target audience of this will be able to figure all of that out. Just saying you need to get credentials seems sufficient.

As for keeping the pony config in one file.. I think thats fine too, so long as its configurable.

The changes look good

@forgottentea-xx
Copy link

Lets make it public! Have you considered writing a blog post about this?

@JacobEvelyn
Copy link
Member Author

Awesome, thanks! Just pulled the trigger. :)

For the time being (until we get more users and a little more buy-in in general) I think we should hold off on publicizing this, but hopefully in the future we can do a blog post and all that!

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

No branches or pull requests

2 participants