Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to Metrics 3.0, update our API to match it #22

Merged
merged 21 commits into from
May 16, 2014
Merged

Conversation

michaelklishin
Copy link
Collaborator

For #19.

I still need to update the docs but the idea I had in mind is implemented and all tests pass. Note that
one small feature (defgauge with a body) and a couple of tests are gone:

  • We generally manage to use the default registry or a user-provided one in the API but it's a real mess with variadic defgauge w/ a body, so I decided to drop it for 2.0.
  • Metrics 3.0 uses nanoseconds precision in timers, which makes some of the tests quite confusing.

@sjl thoughts?

@michaelklishin
Copy link
Collaborator Author

Also, there are some purely stylistic changes that have nothing to do with Metrics 3.0 but it's also
a good chance to move closer to a more widely accepted Clojure code style. I'm not anal about
code styles, though, and can manually revert things if someone cares.

@sjl
Copy link
Collaborator

sjl commented Apr 27, 2014

Match the existing style. If you want to change whitespace to some weird new format I don't really care, but do it all at once in a separate PR.

@michaelklishin
Copy link
Collaborator Author

The problem is, with multi-arity functions the old style isn't really possible. We go from

(defn deftimer [title] …)

to

(defn deftimer
  ([title]
      …)
  ([^MetricRegistry reg title]
      …))

I can move some of the functions to use earlier style, wouldn't it be even less consistent, though?

@sjl
Copy link
Collaborator

sjl commented Apr 27, 2014

Currently things look like this:

(defn foo
  "doc"
  [a]
  bar)

(defn foo
  "doc"
  ([a]
   bar)
  ([b]
   baz))

If you want to add new functions, match this style. Otherwise you'll end up
with multiple indentation styles intermingled within the same file which is just
awful.

Then, in a separate PR afterwords you can add the extra whitespace to ALL
multiple arity functions.

I'm confused why would it be indented four spaces instead of two like everything
else though. Shouldn't it be:

(defn foo
  "doc"
  [a]
  bar)

(defn foo
  "doc"
  ([a]
    bar)
  ([b]
    baz))

@miner
Copy link
Contributor

miner commented Apr 27, 2014

I'm happy to see the upgrade to Metrics 3.0. The multiple arity approach seems like a reasonable compromise to maintain backwards compatibility (mostly) but at the same time support explicit registries.

@michaelklishin
Copy link
Collaborator Author

@sjl this is what the most recent stock clojure-mode indents it like. It shouldn't be 4 spaces, I'll investigate.

@michaelklishin
Copy link
Collaborator Author

@sjl is 3cbce6e better?

@michaelklishin
Copy link
Collaborator Author

@sjl is there anything else you want me to do with this PR?

@jwhitlark
Copy link
Contributor

I'm hoping this is almost ready. I wanted to start using it, but if it's not up-to-date, I'm going to have to just do the java-interop thing on the metrics lib.

@cprice404
Copy link

+1, we are really interested in this and would be happy to help if needed!

@michaelklishin
Copy link
Collaborator Author

@sjl if there are no more comments in the next few days, I'll assume this is good to merge and release as 2.0 :)

@sjl
Copy link
Collaborator

sjl commented May 16, 2014

Yeah, it's fine. Go ahead.

On Thu, May 15, 2014 at 4:41 PM, Michael Klishin
notifications@github.comwrote:

@sjl https://github.com/sjl if there are no more comments in the next
few days, I'll assume this is good to merge and release as 2.0 :)


Reply to this email directly or view it on GitHubhttps://github.com//pull/22#issuecomment-43262065
.

michaelklishin added a commit that referenced this pull request May 16, 2014
Upgrade to Metrics 3.0, update our API to match it
@michaelklishin michaelklishin merged commit 57a8e0a into master May 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants