Add devtools #10

indrekj · 2013-05-06T13:52:55Z

No description provided.

* Make Yardstick::Processor simpler - Delegates work to parser and DocumentSet. * Add Yardstick::Parser - Talks directly with YARD, returns set of documents. * Add Yardstick::DocumentSet - Wrapper for documents.

coveralls · 2013-05-06T13:54:44Z

Coverage remained the same when pulling 6f7a432 on indrekj:devtools into c937f26 on dkubb:master.

coveralls · 2013-05-06T14:23:56Z

Coverage remained the same when pulling 9ab9660 on indrekj:devtools into c937f26 on dkubb:master.

snusnu · 2013-05-06T14:35:58Z

@indrekj you can fix the timeout error on rbx by doing something like: https://github.com/snusnu/substation/blob/master/config/devtools.yml

indrekj · 2013-05-06T14:38:01Z

Build failed because (rbx-18):

1) Yardstick::Rake::Verify#initialize with custom arguments when threshold is not specified raises error
     Failure/Error: Unable to find matching line from backtrace
     Devtools::Project::TimeoutError:
       Unit test took 0.10097199999999999 but max allowed is 0.1

https://github.com/indrekj/yardstick/blob/devtools/lib/yardstick/rake/verify.rb#L33
https://github.com/indrekj/yardstick/blob/devtools/spec/unit/yardstick/rake/verify/initialize_spec.rb#L38

It seems a random error that doesn't happen every time. Any suggestions how to fix it?

indrekj · 2013-05-06T14:38:39Z

Oh, I was late :). Thanks.

mbj · 2013-05-06T14:39:04Z

lib/yardstick/rake/measurement.rb

@@ -43,7 +43,7 @@ def initialize(options = {}, name = :yardstick_measure, &block)
      #
      # @api public
      def yardstick_measure
-        write_report { |io| Yardstick.measure(@config).puts(io) }
+        @config.output.write { |io| Yardstick.measure(@config).puts(io) }


Nitpicking: You introduce a LOD violation with the remove of the #write_report method below.

@indrekj Sorry, for the noise. The removed method was only used once and also has LOD violation. Ignore this ;)

The LoD violation was in the write_report method before I moved it here. Do you think it would be better to move it to config? e.g @config.write_output {}. Or is there a nicer way?

EDIT: oh, I have a lag or something. I didn't see your second message before writing this.

I think for this case it is just acceptable as is.

If there are multiple writes in this class I'd reintroduce that helper method with LOD-violation again. If @config.output is accessed twice I'd add a special proxy method like: def output; @config.output; end with private visibility.

IMHO LOD must be fixed if same navigation (sub) path is used more than once. If you only navigate once it depends on badness of more-loc vs LOD-violation.

Edit:

You could also "unpack" output to an instance variable in initializer.

Travis rbx-18mode failed with: Unit test took 0.10097199999999999 but max allowed is 0.1

coveralls · 2013-05-06T14:42:17Z

Coverage remained the same when pulling c8d3f03 on indrekj:devtools into c937f26 on dkubb:master.

mbj · 2013-05-06T14:46:16Z

spec/integration/yardstick/processor/process_string_spec.rb

+  end
+
+  describe 'without a method summary when validations are turned off' do
+    let(:config) {


Stylistic:

I'd use do end style for blocks spanning multiple lines.

let(:config) do ... end

I'm interested what is the correct convention here. I remember reading from IRC that blocks which return something should always use {}. Not sure if it was just for discussion or an actual convention.

i usually follow the convention you've mentioned rather strictly, therefore let statements that use do ... end look very weird to me

I know the following distinct conventions:

Use do end for multi-line blocks. I think @dkubb and me prefer this.

Use {} for block where the return value is used as an argument or receiver, or as an rvar in an expression. I know @solnic and maybe @snusnu prefer this.

EDIT: distinct

@dkubb I guess you repo, your rules. Is this ( https://github.com/dkubb/styleguide/blob/master/RUBY-STYLE#L55 ) up to date?

@indrekj yeah, it's mostly up to date. I wonder if I should probably change that to something like:

* Use do...end for multiline blocks. Use {...} when defining one-line blocks, or when calling a method on the return value.

@mbj does this summarize your conventions? I think it does with mine.

@dkubb Yeah, its in sync with mine. Lets convince @solnic and @snusnu some time ;)

Travis jruby-19mode failed with: Unit test took 0.253 but max allowed is 0.2

coveralls · 2013-05-06T14:58:03Z

Coverage remained the same when pulling 12d1908 on indrekj:devtools into c937f26 on dkubb:master.

indrekj · 2013-05-06T15:03:17Z

There seems to be something wrong with Travis. I got Unit test took 1.826304 but max allowed is 0.3. Last time it passed with max 0.2. And before that is passed with max 0.1.

mbj · 2013-05-06T15:12:59Z

@indrekj Its just statistic fluctuation. We just discussed this here: http://irclogger.com/.datamapper/2013-05-06#1367851643

IMHO we should pick 0.3 or something now and use a different strategy in future.

snusnu · 2013-05-06T15:15:52Z

@mbj tbh, imo everytime i have to adjust the timeout value to something i have no idea if it will pass or not, i'm just annoyed and it takes away time. i'd vote for deactivating that extra safety net until we have a solution that checks cpu time only.

mbj · 2013-05-06T15:22:57Z

@snusnu Lets disable it for now. I dislike options.

dkubb · 2013-05-06T20:25:37Z

lib/yardstick/rules/summary.rb

@@ -43,7 +43,7 @@ class Length < Rule
        #
        # @api private
        def valid?
-          summary_text.split(//).length <= MAXIMUM_LINE_LENGTH
+          summary_text.length <= MAXIMUM_LINE_LENGTH


I'm not sure this will return the same answer on ruby 1.8 as it would under 1.9 or 2.0. A cross-version approach would probably be:

summary_text.split(//u).length <= MAXIMUM_LINE_LENGTH

Do we need yardstick under 1.8? I don't think so. Specs of our apps should pass, but IMHO there is no reason to add complexity to our tools. Especially as tool result under 1.8 is not relevant.

IMHO we could just drop 1.8 support from all our "tools". Like we did in mutant.

split(//u) works correctly with multibyte characters under 1.8, but I'm not sure how to make mutant pass.

Alive: rspec:Yardstick::Rules::Summary::Length#valid?:/Users/indrek/yardstick/lib/yardstick/rules/summary.rb:45:75b73 (0.11s) @@ -1,4 +1,4 @@ def valid? - ((summary_text.split(//u).length) <= (MAXIMUM_LINE_LENGTH)) + ((summary_text.length) <= (MAXIMUM_LINE_LENGTH)) end

@mbj any ideas?

For 1.8 I'd make a test:

context 'with short summary that includes umlauts' do let(:text) { 'ö' * 79 } it { should be(true) } end

but this passes without split(//u) in 1.9, so the mutant thinks it is not necessary.

@indrekj I have one alternate approach. Since yardstick uses backports, under 1.8 we have String#chars available. We could change that code to:

summary_text.chars.count <= MAXIMUM_LINE_LENGTH

Which should produce the same result in 1.8 and 1.9 and pass through mutant.

@mbj I agree with you in principle, but I'd rather only drop support when the effort to support 1.8 is significant (or impossible in the case of some tools). We're getting closer to that, but I don't think we're there yet.

@indrekj that is odd. I swear I tested it locally under 1.8.7 before suggesting it.

I wonder if this will work:

'ö'.split(//u).count

Also, do you mind submitting an issue to backports about #chars behaviour being different? I don't know if @marcandre would consider it a bug but it's worth passing along just in case.

It is very unlikely we'll have multibyte chars in the description. So why not stick to String#length here? It might return incorrect values under 1.8, but the likeliness it triggers user fancing misbehavior is low.

Sorry for reposting, but I posted out of thread.

Yes, split(//u) worked correctly, but I had trouble testing it with mutant.

@indrekj the difference is we're calling #count not #length. That should allow it to pass with mutant because when mutant removes the split(//u) statement an exception will be thrown, causing the mutation to be killed.

@dkubb Yes, you're right. I did not think about it like that :).

coveralls · 2013-05-09T21:34:04Z

Coverage remained the same when pulling c445462 on indrekj:devtools into c937f26 on dkubb:master.

solnic · 2013-05-10T07:37:00Z

@dkubb @mbj please excuse my snarky comment but...this conversation seems to be a significant effort. I wholeheartedly agree with @mbj that adding complexity to a tool that could work only in 1.9 mode is a wasted effort. yardstick is such a tool, there's no good reason why it must work under 1.8. People could just use it under > 1.9 (just like we run all our metric stuff only under 1.9.3 these days).

We talked about 1.8 support plenty of times but months are passing and we're still wasting time trying to support it with very little to none benefit. I know there are many apps out there running under 1.8 but at the same time there are many apps running under 1.9 and more apps that will run under 1.9 or 2.0 are being under development. I think we should focus our efforts on the future instead of trying to be nice to people being stuck on 1.8. Why? Because I'm not so sure if those people will ever benefit from using our libs, they have probably bigger problems to solve, like...porting their apps to ruby 2.0.

I'm just saying supporting 1.8 is a significant effort (time wise) and it's simply not worth the price because benefits coming from 1.8 support are too small these days.

...but yeah, your repo your rules. Just me 2 cents :)

coveralls · 2013-05-10T12:53:25Z

Coverage remained the same when pulling c4138a7 on indrekj:devtools into c937f26 on dkubb:master.

indrekj · 2013-05-18T14:28:29Z

Is there anything else I should change here?

dkubb · 2013-05-18T17:49:37Z

@indrekj thanks for pinging me on this. It's awesome. I will merge this now.

Add devtools

mbj · 2013-05-18T17:58:07Z

@indrekj I'm already using your changes in private projects. Thx!

dkubb · 2013-05-18T20:21:48Z

@indrekj I think I'm going to do a yardstick release with your new changes very shortly. I'll probably bump the minor number this time.

Thanks again!

mbj · 2013-05-18T20:53:51Z

@dkubb Thx. I was about to ask ;)

indrekj · 2013-05-18T21:09:11Z

At the moment the exclude feature only works with instance methods and class methods. Tomorrow, I'll will create a pull request so that it also supports classes and modules. This doesn't affect making a release of course. Just letting you know.

dkubb · 2013-05-18T22:06:28Z

@indrekj that sounds good. I'll do a point release after that unless you beat me to it.

indrekj added 14 commits May 5, 2013 16:24

Add devtools

5a6378e

Get most of the metrics passing

6b38022

Add config/mutant.yml

568420d

Add missing specs for Yardstick::Config

2e3b1d2

Add missing specs for Yardstick::Document

61e0497

Add missing specs for Yardstick::MeasurementSet

ff84ad4

Add missing specs for Yardstick::OrderedSet

11a749b

Add missing specs for Yardstick::Processor

762612b

* Make Yardstick::Processor simpler - Delegates work to parser and DocumentSet. * Add Yardstick::Parser - Talks directly with YARD, returns set of documents. * Add Yardstick::DocumentSet - Wrapper for documents.

Add missing specs for Yardstick::Rule and Yardstick::Rules

ec9ef56

Add missing specs for Yardstick::DocumentSet

756b11c

Add missing specs for Yardstick::Rake tasks

abdf8a8

Add missing specs for Yardstick::Parser

cf21209

Add missing specs for Yardstick::ReportOutput

bfed9a7

Add missing specs for Yardstick

6f7a432

Fix Yardstick::Processor#process spec

9ab9660

mbj reviewed May 6, 2013
View reviewed changes

Increase unit_test_timeout a bit

c8d3f03

Travis rbx-18mode failed with: Unit test took 0.10097199999999999 but max allowed is 0.1

mbj reviewed May 6, 2013
View reviewed changes

Increase unit_test_timeout a bit

12d1908

Travis jruby-19mode failed with: Unit test took 0.253 but max allowed is 0.2

dkubb reviewed May 6, 2013
View reviewed changes

Ensure summary length is validated correctly

c445462

indrekj added 2 commits May 10, 2013 15:49

Remove spec/support/config_alias.rb

7518e86

Fix coding style

c4138a7

dkubb added a commit that referenced this pull request May 18, 2013

Merge pull request #10 from indrekj/devtools

a9624da

Add devtools

dkubb merged commit a9624da into dkubb:master May 18, 2013

Add devtools #10

Add devtools #10

Conversation

indrekj commented May 6, 2013

coveralls commented May 6, 2013

coveralls commented May 6, 2013

snusnu commented May 6, 2013

indrekj commented May 6, 2013

indrekj commented May 6, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 6, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 6, 2013

indrekj commented May 6, 2013

mbj commented May 6, 2013

snusnu commented May 6, 2013

mbj commented May 6, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 9, 2013

solnic commented May 10, 2013

coveralls commented May 10, 2013

indrekj commented May 18, 2013

dkubb commented May 18, 2013

mbj commented May 18, 2013

dkubb commented May 18, 2013

mbj commented May 18, 2013

indrekj commented May 18, 2013

dkubb commented May 18, 2013