search

deadtrickster / prometheus.erl

Prometheus.io client in Erlang
MIT License
341 stars 117 forks source link

Tighten up style, add several type specs, appease dialyzer #9

Closed yurrriq closed 8 years ago

yurrriq commented 8 years ago

This is not yet ready to merge, but I figured I'd open a PR so we can discuss these changes as they're being written.

This PR means to address #5.

deadtrickster commented 8 years ago

Wow, this is huge :elephant: !

deadtrickster commented 8 years ago

If you have a goal to fit every line to 80 chars limit there is elvis.config entry to update - https://github.com/deadtrickster/prometheus.erl/blob/master/elvis.config#L8

yurrriq commented 8 years ago

80 chars limit

Only if you're into it. I prefer it, but it's not my project :smile:

yurrriq commented 8 years ago

Ok, I'm doing an elvis overhaul after all...

deadtrickster commented 8 years ago

I refactored register/deregister stuff: https://github.com/deadtrickster/prometheus.erl/commit/1d092bb4ef10a917c95a2a48db87db34168f24d6. And this means more conflicts :-(.

yurrriq commented 8 years ago

I'm resolving the conflicts now in pieces.

yurrriq commented 8 years ago

Ok. This turned into more of a style overhaul, but in the end, we've refactored the (de)registration process, added several type specs, appeased dialyzer, smashed everything into < 80 chars wide and I've gotten to know the codebase :)

yurrriq commented 8 years ago

This doesn't fully solve the type spec issue, but I'd say this is ready to merge.

yurrriq commented 8 years ago

The error on Travis looks like a problem wrt downloading R19.

yurrriq commented 8 years ago

I've rebased, reordered, and squashed this into only eight commits. Is that enough or should I squash more?

deadtrickster commented 8 years ago

Nope, I'm merging it.