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

Code enhancements :) #4

Merged
merged 4 commits into from Apr 1, 2018

Conversation

Projects
None yet
5 participants
@faustinoaq
Copy link
Contributor

faustinoaq commented Apr 1, 2018

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 added some commits Apr 1, 2018

@faustinoaq

This comment has been minimized.

Copy link
Contributor Author

faustinoaq commented Apr 1, 2018

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 πŸ˜‰

@faustinoaq faustinoaq referenced this pull request Apr 1, 2018

Closed

Port btest to D #3

1 of 3 tasks complete
@bew

This comment has been minimized.

Copy link

bew commented Apr 1, 2018

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.

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

This comment has been minimized.

@bew

bew Apr 1, 2018

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
Copy link

bew left a comment

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)

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

if !res.success?

This comment has been minimized.

@bew

bew Apr 1, 2018

unless res.success?

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

This comment has been minimized.

@bew

bew Apr 1, 2018

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

less useless allocations πŸ˜ƒ

(ditto bellow)

{ |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|

This comment has been minimized.

@bew

bew Apr 1, 2018

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

@name = Suite.path_to_name(config, path)

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

This comment has been minimized.

@bew

bew Apr 1, 2018

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

This comment has been minimized.

Copy link
Owner

briansteffens commented Apr 1, 2018

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

This comment has been minimized.

Copy link
Contributor Author

faustinoaq commented Apr 1, 2018

@bew Thank you for your help!

@briansteffens Thank you so much for your comment! πŸ˜„

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

This comment has been minimized.

@bew

bew Apr 1, 2018

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

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

This comment has been minimized.

@bew

bew Apr 1, 2018

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

This comment has been minimized.

@faustinoaq

faustinoaq Apr 1, 2018

Author Contributor

Yeah, looks like a bug πŸ˜…

This comment has been minimized.

@faustinoaq

faustinoaq Apr 1, 2018

Author Contributor

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

This comment has been minimized.

Copy link
Collaborator

eatonphil commented Apr 1, 2018

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

This comment has been minimized.

Copy link
Contributor Author

faustinoaq commented Apr 1, 2018

@bew Thank you again! πŸ‘

@eatonphil No problem, Thank you for your comment! πŸ˜„

@briansteffens So, should I close this? πŸ˜…

@RX14

This comment has been minimized.

Copy link

RX14 commented Apr 1, 2018

@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

This comment has been minimized.

Copy link
Owner

briansteffens commented Apr 1, 2018

@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

This comment has been minimized.

Copy link
Owner

briansteffens commented Apr 1, 2018

@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.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.