Skip to content

Commit

Permalink
add a VERSION constant (#471)
Browse files Browse the repository at this point in the history
  • Loading branch information
@lebogan
lebogan authored and Serdar Dogruyol committed Jul 16, 2018
1 parent 1e18389 commit 09bb1fc
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 0 deletions.
8 changes: 8 additions & 0 deletions spec/config_spec.cr
View file
Expand Up @@ -54,4 +54,12 @@ describe "Config" do
Kemal::CLI.new
test_option.should eq("FOOBAR")
end

it "returns a string" do
Kemal::VERSION.should be_a(String)

This comment has been minimized.

Sign in to view

Copy link
@straight-shoota

straight-shoota Jul 17, 2018

Contributor

Speccing the type of a constant is pretty much unnecessary.
end

it "gets the version from shards.yml" do
Kemal::VERSION.should eq("0.23.0")

This comment has been minimized.

Sign in to view

Copy link
@Sija

Sija Jul 17, 2018

Contributor

This will fail in the very next release, more sensible would be to parse the shard.yml file and use that.

This comment has been minimized.

Sign in to view

Copy link
@straight-shoota

straight-shoota Jul 17, 2018

Contributor

This wouldn't be useful at all because it would use more or less the same implementation as the constant value that is to be testes.

There shouldn't be any spec for a specific version value. Just make sure it's not empty.

This comment has been minimized.

Sign in to view

Copy link
@Sija

Sija Jul 18, 2018

Contributor

or matches \d+\.\d+\.\d+(\.\d+)?

This comment has been minimized.

Sign in to view

Copy link
@straight-shoota

straight-shoota Jul 18, 2018

Contributor

Guys, don't over engineer it. A version must be present, that's all. There should not be any hard coded value or format expectations. It's pretty much useless. Better work on something that actually improves things.
end
end
2 changes: 2 additions & 0 deletions src/kemal/config.cr
View file
@@ -1,4 +1,6 @@
module Kemal
VERSION = {{ `shards version #{__DIR__}`.chomp.stringify }}

# Stores all the configuration options for a Kemal application.
# It's a singleton and you can access it like.
#
Expand Down

5 comments on commit 09bb1fc

@lebogan
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, don't waste any more time on this. The PR was intended to be 'cosmetic'. It adds no real value to the code base. Kemal is probably the coolest thing since Sinatra. Please, focus on making it even cooler.

Thank you all.

@Sija
Copy link
Contributor

@Sija Sija commented on 09bb1fc Jul 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[...] The PR was intended to be 'cosmetic'. It adds no real value to the code base. [...]

@lebogan Well, it adds value in a sense of a 🕦💣 (ticking bomb) waiting to explode on CI at very next release. I know what I'd do in your position: I'd send a followup PR fixing this.

@sdogruyol
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh god..I'm on a business trip and really shocked to see this much bikeshedding going on for this...if we don't like it or it breaks..we can just revert it

@RX14
Copy link

@RX14 RX14 commented on 09bb1fc Jul 18, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll update the version number in a PR, and if you forget to update the specs CI will fail. Thats two places to update the number but CI will enforce that. So who cares, this commit is fine and is exactly how i'd do it.

@Sija
Copy link
Contributor

@Sija Sija commented on 09bb1fc Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sdogruyol Bikeshedding? Yes, if you mean all that needless conversation for something that shouldn't be there in the first place. No, if you mean added complexity for nothing.

@RX14 I haven't seen anywhere else one example of such, ermm... creative approach for making life harder for yourself and other contributors. Can't wait for the comments on next releases, where from now on you'll have not one but two places to update the version string (and it just got automated in VERSION constant...)

Please sign in to comment.