Crystal: [RFC] Remove spec context

Created on 8 Apr 2019  ·  10Comments  ·  Source: crystal-lang/crystal

With the emphasis on removing aliases in Crystal, I propose removing the context method. It is essentially an alias of describe. While I kinda get what it's meant for, the actual helpfulness of it is less so. There is not really any benefit in using context "with an empty array" do instead of describe "with an empty array" do since the context is implied from the description. The different keywords only adds confusion to figure out which one would make the most sense.

Most helpful comment

The actual implementation is identical (or in fact, an alias) but it's not just two different names for the same thing. In the spec framework describe and context have different semantics.

Whether we want these semantic difference can be questioned, but it shouldn't matter that the implementation happens to be the same.

I prefer to have both, describe and context because it helps writing more expressive specs. The amount of confusion is negligible. The concepts should be pretty much self-explanatory for anyone having a little knowledge about writing specs.

All 10 comments

I disagree. You're describing something. And that description can have many contexts.

I don't think @vladfaust's argument is enough, because one could justify any alias like that

The actual implementation is identical (or in fact, an alias) but it's not just two different names for the same thing. In the spec framework describe and context have different semantics.

Whether we want these semantic difference can be questioned, but it shouldn't matter that the implementation happens to be the same.

I prefer to have both, describe and context because it helps writing more expressive specs. The amount of confusion is negligible. The concepts should be pretty much self-explanatory for anyone having a little knowledge about writing specs.

I think this is a very valid point. If the case of "an alias is ok if the method allows for expressiveness" becomes a rule, then how are we to know which aliases are ok? I'd say in this PR, for me Time.now is a lot more expressive than Time.local. Even @ysbaddaden brings up the point of wishing we'd keep a few aliases because some aliases can be helpful to be more expressive.

I'm not opposed to keeping both describe and context because I use both, but I think whatever the "rule of thumb" is for the language, it should be consistent across all parts of the language. This could be done just with some simple internal documentation like # SEMANTIC ALIAS or something.

Well, IMHO context and description is like this:

name --> meaning -+
                   \
                    +-> implementation
                   /
name --> meaning -+

While Time.local and Time.now is like this:

name -+
       \
        +-> meaning --> implementation
       /
name -+

I'm not opposed to keeping Time.now indefinitely as well. But it's a hard bargain. It's an alias after all and there is no benefit for being able to call two different methods that mean the same thing. It's not like you would use Time.now in specific situations and Time.local in others (probably... maybe someone might actually do that 🤷‍♂️ but that's not what it's intended for).

@straight-shoota people are used to Time.now, its understandable. Letting the deprecated warning long enough to teach everybody to use Time.local, then we'll be able to remove the alias :+1:

Me personally, I do use them in different situations, but Time stuff aside, my point is more how can we define when an alias is ok, and when is it not? Like in this case, context and description are in fact an alias, but we say it's ok because of their semantic meaning being different. If that's the case, we (as a community) should have some way to visibly see that. So if we had some sort of comment tag like :nodoc: :semantic: or whatever that said "this alias is ok" vs someone submitting a PR to remove context because they saw a chance for a cheap PR to remove an alias since the rule is "no aliases".

I don't think we need a way to declare this. It should be evident from the documentation. We're just lacking that for description and context 😆
I trust in the competence of maintainers and experienced contributers to point out the error of such cheap PRs.

The idea of removing some aliases from the stdlib was:

  • avoid having different styles (some prefer map, some prefer collect and that's a style clash)
  • avoid having to implement aliases in more containers (if we have array.first(10) and array.take(10) you'll have to implement those aliases for containers that behave like Array and that's a bit annoying)

For spec, describe and context are probably fine. It's not like you prefer one or another, they convey a different meaning: describe to describe an object or some feature, context to test some things in a given context. It's different to an alias like collect for map because then it becomes a user preference, but context and describe are not a preference.

Seems consensus is to keep it. I'll go ahead and close this.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

grosser picture grosser  ·  3Comments

ArthurZ picture ArthurZ  ·  3Comments

Sija picture Sija  ·  3Comments

cjgajard picture cjgajard  ·  3Comments

pbrusco picture pbrusco  ·  3Comments