search

heroku / heroku-connect-plugin

CLI plugin for Heroku Connect (experimental)
Apache License 2.0
23 stars 8 forks source link

Testing and other Hygiene #62

Closed sigmavirus24 closed 7 years ago

sigmavirus24 commented 7 years ago

This is a large-ish pull request which starts by using standard to lint our plugin and then re-organizes the plugin to match the canonical style used by other Heroku-maintained plugins. Finally, it starts adding tests for the plugin so that we can verify it will work and improve maintainability. A separate pull request should configure a CI service (Travis, Circle, etc.).

gulopine commented 7 years ago

I'll do a more detailed review at some point, but immediately I'm reminded of how much I detest the JavaScript's relatively recent fear of semicolons. I get that they can be excluded in many (most?) cases, but given that it's not universal, and I don't want to worry about when they are/aren't required, I've never considered omitting them to be good style. I admit I'm not a JavaScript guy, so I'm willing to go along with that and see how it goes, but I figure it's worth speaking up in case anybody else has similar opinions.

sigmavirus24 commented 7 years ago

I admit I'm not a JavaScript guy, so I'm willing to go along with that and see how it goes,

I'm not either. I was using the linter suggested by heroku's devcenter plugin articles. It auto-fixes things like this. If it will work the opposite way, I'm happy with that. I really don't care either way. I just want us to have some measure of consistency.

iamjem commented 7 years ago

Maybe we should use the same linting config we use on our other projects? @hburrows might be best to point to the correct place to pull from... but dashboardv2 has a .eslintrc, probably just remove the react related bits.

gulopine commented 7 years ago

I think @sigmavirus24's point is fair that as long as Heroku publishes CLI plugin guidelines, we should probably follow those, for the sake of better sharing code with other plugins, soliciting help/input from plugin authors, etc. It'll tae some getting used to, but we'll manage.

sigmavirus24 commented 7 years ago

@gulopine I don't think semi-colons will make any of this harder though. To me, this is a discussion much like the typical Flake8 max-line-length discussion. Yes it defaults to what the original value in the PEP was, but that doesn't prevent teams from adjusting the tool to their own preferences/agreed upon standards. If our team has an eslintrc that codifies our team's opinions about JS, that's fine by me. I hadn't honestly looked in our private repo for that. As long as we apply those same standards everywhere, I'm happy to adopt that here. Some consensus would be helpful though. :)

gulopine commented 7 years ago

👍 to this as-is. Things like camelCase may not be my general preference, but it is appropriate for JavaScript, and it's certainly better to be consistent around something. Semicolons aren't gonna be a showstopper for me either way.

As for using an .eslintrc, it seems we have several. Each JavaScript thing we have (dashboard, node-proxy, event_notifier, etc) has its own. They're probably all pretty similar, but I expect they all came from using a base that was customized for each project. Any idea which of those customizations make sense here? Or do we want to just leave this as-is until some of its choices bother us enough to make a change?

sigmavirus24 commented 7 years ago

Things like camelCase may not be my general preference

Me too. I am not a camelCase person. This is one of perhaps the least significant reasons I like Rust over Go, Python and Ruby over Java, etc.

Or do we want to just leave this as-is until some of its choices bother us enough to make a change?

As I'm a bit lazy, I'm in favor of letting this simmer for a bit before we make any further changes. I'd really like to get CI set-up for this repo sooner rather than later and that kind of necessitates this PR being merged. In that vein, I'm going to take the +1 and run with it. :)