Code enhancements :) #4

faustinoaq · 2018-04-01T03:46:28Z

Hi @briansteffens @eatonphil I just seen your tweets and I really like you tried crystal, a bit sad you found some issues while using it 😅

I already opened an issue trying to remove the barrier for crystal newcomers, see: crystal-lang/crystal#5291

I tested your code and I think you can do some enhancements. By example crystal has a good code guideline and a nice integrated formatter (crystal tool format). Also we have ameba, a nice linter to make things even better 😉

I tried to use getter macros and some nice crystal features that you're not using at all, like implicit var initializer.

BTW, I'm still using 79 characters per line and I didn't remove the extra comment lines, (you have more comments lines in crystal than D)

I think you should allow more than 79 characters per line but hey! every developer has his own style 👍

Cloc output comparing D vs Crystal code (Your PR vs My PR)

➜  code cloc .
       2 text files.
       2 unique files.                              
       0 files ignored.

github.com/AlDanial/cloc v 1.74  T=0.02 s (97.9 files/s, 35471.1 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Crystal                          1             87             28            293
D                                1             50              5            262
-------------------------------------------------------------------------------
SUM:                             2            137             33            555
-------------------------------------------------------------------------------

And finally, I'm not a crystal expert, I still think this code can be reduced even more 😅

/cc @RX14 @straight-shoota

faustinoaq · 2018-04-01T04:14:05Z

BTW, ((293-262)/262)*100 = 11.83206106870229

So actually this PR isn't 25% more lines than D but just 12 %

and if you remove the comments and some blank lines you can get it very close 😉

bew · 2018-04-01T05:13:29Z

https://github.com/briansteffens/btest/pull/4/files#diff-7f6cbc5933f03b2aeb6354899dd13e7fR186
if ["status", "stdout", "stdout_contains"].includes? key
Here you can use a tuple instead of an array, to avoid useless memory allocations.

bew · 2018-04-01T05:21:31Z

src/btest.cr

-                "Status code: #{res.exit_code}\n" \
-                "Standard output: #{stdout.to_s}\n" \
-                "Standard error: #{stderr.to_s}\n")
+          "Error running: #{cmd2}\n" \


You could use a string literal here for the output text:

Result.new(runner, self, false, <<-OUTPUT Error running: #{cmd2} Status code: #{res.exit_code} Standard output: #{stdout.to_s} Standard error: #{stderr.to_s} OUTPUT )

bew

Some little things here and there, for a first pass on your rework 😃

There are some other things you could do for better performance, for example, use an IO::Memory to generate the Result's output, this would allow to do a few memory allocations compared to the tons of allocations for all the strings concatenations.

Also, question for the original dev, why Case doesn't have a YAML.mapping ? (instead of using YAML.parse then doing things manually as of now)

bew · 2018-04-01T05:22:11Z

src/btest.cr

@@ -280,10 +231,10 @@ class Case

      if !res.success?


unless res.success?

bew · 2018-04-01T05:29:04Z

src/btest.cr

-    suite = "#{@testCase.suite.name}".colorize(:white)
-    runner = "#{@runner.name}".colorize(:dark_gray)
-    ret = "#{runner} #{suite} #{name}"
+  suite = "#{@testCase.suite.name}".colorize(:white)


you don't need the string interpolation here: @testCase.suite.name.colorize(:white)

less useless allocations 😃

(ditto bellow)

bew · 2018-04-01T05:30:56Z

src/btest.cr

-    { |t| arg_threads = t.to_i64 }
-  parser.on("-h", "--help", "Show this help") \
-    { puts parser; Process.exit(0) }
+  parser.on("-s SUITE", "--suite", "Run only this suite") { |s|


use do ... end instead of { ... } as you go multiline, it feels better on the eye (at least for me ... and the convention!)

bew · 2018-04-01T05:38:05Z

src/btest.cr

    @name = Suite.path_to_name(config, path)

    begin
-        file = YAML.parse(File.read(path))
+      file = YAML.parse(File.read(path))


Use File.open instead of File.read, so the YAML parser can read directly from the file instead of reading the file first (allocating a (big?) string), then parsing the string.

briansteffens · 2018-04-01T19:57:18Z

Hey @faustinoaq and @bew - this is really cool, thanks for reviewing my crappy crystal code! I appreciate the advocacy for the language you contribute to, and it makes the crystal community look good to see people who care about it.

To give a bit more information, btest is a utility used by a couple of side projects, which are both written in D. Since both are D, it would simplify the build process for our side projects to have fewer dependencies and fewer different languages involved.

I've generally liked crystal so far. I think the only big deal-breaker for me right now is the lack of threading support. This project gets around it by launching processes, but that won't work for all projects :)

I haven't had a chance to take a close look or compare this with the @eatonphil D version but I'll try to get to it today. At quick glance, this looks like a lot of nice improvements. Love the getter macros in particular.

faustinoaq · 2018-04-01T21:24:01Z

@bew Thank you for your help!

@briansteffens Thank you so much for your comment! 😄

bew · 2018-04-01T21:35:19Z

src/btest.cr

-          "Standard error: #{stderr.to_s}\n")
+      unless res.success?
+        Result.new(runner, self, false, <<-OUTPUT
+        Error running: #{cmd2}


Could you indent a bit here please? It feels easier to follow when it's indented

bew · 2018-04-01T21:41:53Z

src/btest.cr

-      name = name[0..name.size - chop_delta]
-      uncolored_len -= chop_delta - 1
-    end
+  # Chop the output if the name is too long


Why is there indentation changes here? (maybe a bug in the formatter?)

Yeah, looks like a bug 😅

Yeah, is a bug

Minimal sample:

def foo bar = "" bar = "foo" + \ "bar" bar end

I guess is fixed on master, no? see: crystal-lang/crystal#5892 (comment)

eatonphil · 2018-04-01T21:56:12Z

It's awesome to see you guys pitching in like this. A little after posting the tweet that may have brought you here (and after a week spent porting this project to D 😨 ), I realized that there is a bug in btest that miscalculates the width of the terminal while it tries to print. Somehow this only manifested when I moved from TravisCI to CircleCI (both of which are dockerized environments). So the bug is not in Crystal. Somehow in the CircleCI environment the miscalculation is such that btest wrote over 3mb of data to stdout (understandable how slow this could have been). It also crashed CircleCI's UI rendering the 3mb out.

In any case, Brian and I talked about the port to D being reasonable esp since both our projects using it are also written in D. Any benefits in speed or code reduction were small side-benefits. (We never had issues with Crystal performance before accidentally trying to shove 3mb of data through it to stdout.)

That said, if I've got some Crystal folks attention, I'll point out a few issues that I do have running btest or BSDScheme. Installing Crystal is a PITA -- primarily in Dockerized test environments. It's understandably difficult to get a package into Debian/Ubuntu official repos... You basically just have to be stable and have existed for many years. But in Docker-land (where this sort of automated testing takes place), having a Docker image for running Crystal would be a big help. Honestly I even had some trouble getting D installed and configured correctly in Docker until I used their image that provided the compiler and package manager.

Being able to statically compile Crystal binaries might also help for Dockerized environments (especially minimal ones like Alpine-based images).

Brian pointed out that lack of threading in Crystal is an issue. In a CLI like this it is perhaps more important than in a server environment where startup time is not an issue.

In any case, thanks a ton for you guys taking a look at btest :) It speaks well of the community. I don't think Crystal is a good choice for this project (mostly to make it easier to maintain w.r.t. our primary projects, bsdscheme and bshift) but I cannot say anything particularly negative about Crystal's performance.

faustinoaq · 2018-04-01T22:30:24Z

@bew Thank you again! 👍

@eatonphil No problem, Thank you for your comment! 😄

@briansteffens So, should I close this? 😅

RX14 · 2018-04-01T23:07:25Z

@eatonphil we do have an official docker image: https://hub.docker.com/r/crystallang/crystal/

It's based on the existing debian packages which have been statically compiled since 0.24.1.

However if you're working in D, then it does make a lot of sense to write your infrastructure in D.

briansteffens · 2018-04-01T23:14:54Z

@faustinoaq Don't close it :) I'm probably going to merge it for now. The D port isn't quite ready anyway and this is definitely an improvement over what's in master.

briansteffens · 2018-04-01T23:20:24Z

@bew I wasn't aware of YAML.mapping, looks like a much better way to parse this, thanks for mentioning it.

Edit: Nevermind, I was confused! In that situation I wanted to put status, stdout, and stdout_contains in the @expect hash and put all others in the @arguments hash. So there's a bit of a transformation between the YAML schema and the in-memory schema, and I wasn't sure how to do that with a YAML.mapping.

faustinoaq added 2 commits March 31, 2018 22:32

Code enhancements :)

a595b29

Revert deleted comment

a736458

faustinoaq mentioned this pull request Apr 1, 2018

Port btest to D #3

Closed

3 tasks

bew reviewed Apr 1, 2018

View reviewed changes

Apply suggestions

51af51b

bew reviewed Apr 1, 2018

View reviewed changes

Fix indentation (almost)

9b79e3d

briansteffens merged commit acc70f4 into briansteffens:master Apr 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code enhancements :) #4

Code enhancements :) #4

faustinoaq commented Apr 1, 2018 •

edited

Loading

faustinoaq commented Apr 1, 2018 •

edited

Loading

bew commented Apr 1, 2018

bew Apr 1, 2018 •

edited

Loading

bew left a comment •

edited

Loading

bew Apr 1, 2018

bew Apr 1, 2018 •

edited

Loading

bew Apr 1, 2018 •

edited

Loading

bew Apr 1, 2018

briansteffens commented Apr 1, 2018

faustinoaq commented Apr 1, 2018

bew Apr 1, 2018

bew Apr 1, 2018

faustinoaq Apr 1, 2018

faustinoaq Apr 1, 2018 •

edited

Loading

eatonphil commented Apr 1, 2018

faustinoaq commented Apr 1, 2018

RX14 commented Apr 1, 2018

briansteffens commented Apr 1, 2018

briansteffens commented Apr 1, 2018 •

edited

Loading

Code enhancements :) #4

Code enhancements :) #4

Conversation

faustinoaq commented Apr 1, 2018 • edited Loading

faustinoaq commented Apr 1, 2018 • edited Loading

bew commented Apr 1, 2018

bew Apr 1, 2018 • edited Loading

Choose a reason for hiding this comment

bew left a comment • edited Loading

Choose a reason for hiding this comment

bew Apr 1, 2018

Choose a reason for hiding this comment

bew Apr 1, 2018 • edited Loading

Choose a reason for hiding this comment

bew Apr 1, 2018 • edited Loading

Choose a reason for hiding this comment

bew Apr 1, 2018

Choose a reason for hiding this comment

briansteffens commented Apr 1, 2018

faustinoaq commented Apr 1, 2018

bew Apr 1, 2018

Choose a reason for hiding this comment

bew Apr 1, 2018

Choose a reason for hiding this comment

faustinoaq Apr 1, 2018

Choose a reason for hiding this comment

faustinoaq Apr 1, 2018 • edited Loading

Choose a reason for hiding this comment

eatonphil commented Apr 1, 2018

faustinoaq commented Apr 1, 2018

RX14 commented Apr 1, 2018

briansteffens commented Apr 1, 2018

briansteffens commented Apr 1, 2018 • edited Loading

faustinoaq commented Apr 1, 2018 •

edited

Loading

faustinoaq commented Apr 1, 2018 •

edited

Loading

bew Apr 1, 2018 •

edited

Loading

bew left a comment •

edited

Loading

bew Apr 1, 2018 •

edited

Loading

bew Apr 1, 2018 •

edited

Loading

faustinoaq Apr 1, 2018 •

edited

Loading

briansteffens commented Apr 1, 2018 •

edited

Loading