thoughtbot / carnival

An unobtrusive, developer-friendly way to add comments
MIT License
501 stars 30 forks source link

Delete demo comments after one week #273

Closed jferris closed 9 years ago

jferris commented 9 years ago

Comments on the demo site are for the purpose of trying out the site. We expect a lot of "test and "hello" type of comments, and the page will look strange with hundreds of old test comments on it.

This commit adds a subcommand to the carnival executable to delete stale comments from the demo site.

jferris commented 9 years ago

cc @pbrisbin @thoughtbot/haskell

jferris commented 9 years ago

@pbrisbin I did a pass adding another executable instead of adding a subcommand: https://github.com/thoughtbot/carnival/commit/8728a62e17f833e59c58620d575c4fc8d9372aba

Some observations:

Given the small size of our starting slug, I think having two executables is okay. If we continue to add tasks, though, I don't think we'll want to add an executable for each one.

I deployed the separate-executable version to staging, and it seems to pick up the application config correctly. For example, it connected to the staging database and correctly deleted old comments from the demo site on staging.

pbrisbin commented 9 years ago

That all sounds great. To clarify, I wouldn't advocate a separate executable for each task, I'd advocate two executables: one to run the app, and one (with sub-commands) to run any tasks.

jferris commented 9 years ago

To clarify, I wouldn't advocate a separate executable for each task, I'd advocate two executables: one to run the app, and one (with sub-commands) to run any tasks.

Yeah, I wanted to see what it would be like to just add a new executable for each one, Mike Burns Style.

I'll mess with that other commit to put in a framework for adding subcommands.

pbrisbin commented 9 years ago

Ha. I would use mbx (Mike Burns Unix).

jferris commented 9 years ago

Okay, I pushed another commit. I'm pretty happy with how this is shaping up.

There's now a carnival-task executable which accepts subcommands. I added a TaskMain and taskParser, which uses optparse-applicative to transform arguments into a Handler ().

I think it will be easy to add tasks in the future, and feels pretty normal, like adding new Yesod routes. You just add a parser step instead of a route and then write a Handler as normal.

pbrisbin commented 9 years ago

LGTM -- having tasks be Handler is strange to me, but I get why. Do be aware that handler does eventually call a function with "unsafe" in its name -- though, I think we know what we're doing here.

jferris commented 9 years ago

LGTM -- having tasks be Handler is strange to me, but I get why. Do be aware that handler does eventually call a function with "unsafe" in its name -- though, I think we know what we're doing here.

I played around for a while trying to find less specific type than Handler. The biggest issue seems to be that methods based around getting the app settings like getYesod only work within handlers.

It seems like it would be possible to have a type that represents an action within the app settings and with a database connection, but without worrying about requests/responses. However, I think that's out of the scope of this pull request.

I'm going to merge this as-is, but I'd be interested in exploring the Handler type more during the next project night.