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

Remove :nodoc: from singular method aliases, add Int#microsecond alias #6297

Merged
merged 1 commit into from Sep 24, 2018

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Jun 29, 2018

Refs #6273

@j8r
Copy link
Contributor

j8r commented Jun 29, 2018

@Sija do you realize this aliases are only one character different?

@asterite
Copy link
Member

@j8r It's about being able to write sleep 1.minute instead of sleep 1.minutes.

@Sija
Copy link
Contributor Author

Sija commented Jun 29, 2018

@j8r yes, I do, see @asterite's comment.

Copy link
Contributor

@j8r j8r left a comment

Choose a reason for hiding this comment

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

The ditto descriptions are bad.
The generated API docs sorts methods alphabetically.
Therefore we will have:

  • #ago, which has a proper description
  • #day, which have ditto as a description
  • #days, which has a proper description.

I don't see better descriptions than "Like #days but without an s" , which is bad 😶
The same applies with the other methods.

@asterite
Copy link
Member

asterite commented Jun 29, 2018

@j8r ditto is processed at the semantic level. That the methods are sorted alphabetically doesn't mean the ditto will be applied like that.

@j8r
Copy link
Contributor

j8r commented Jun 30, 2018

Sorry @asterite this wasn't as clear as :nodoc:, which has colons. Adding them around :ditto: will make more obvious that its a special semantic.

@RX14
Copy link
Contributor

RX14 commented Jun 30, 2018

Yeah I considered that, but it's such a minor change that it's really not worth breaking.

@j8r
Copy link
Contributor

j8r commented Jul 1, 2018

@RX14 :ditto: seems to be already supported here, and also nodoc (without colons) here.
If yes, ditto can already be changed to :ditto:.
I can send a PR about removing the non-colons version of ditto and nodoc.

@RX14
Copy link
Contributor

RX14 commented Jul 2, 2018

:ditto: and nodoc are unused in my experience, ditto and :nodoc: are what people tend to use. I'm not sure thats worth changing.

@straight-shoota
Copy link
Member

@RX14 @j8r please discuss this in a separate issue. Thanks 👍

@RX14
Copy link
Contributor

RX14 commented Jul 2, 2018

Just needs review

@RX14 RX14 requested a review from bcardiff July 2, 2018 21:30
@j8r
Copy link
Contributor

j8r commented Jul 10, 2018

We can use a single macros to add all this non s variants:

{% for time in %w(nanosecond microsecond millisecond second minute hour day week month year)  %}
 # Returns a `Time::Span` of `self` {{time.id}}s.
  def {{time.id}} : Time::Span
    {{time.id}}s
  end
{% end %}

@Sija
Copy link
Contributor Author

Sija commented Aug 17, 2018

Can this go forward?

@Sija
Copy link
Contributor Author

Sija commented Sep 23, 2018

2nd review, @bcardiff @asterite, anyone?

@bcardiff bcardiff merged commit 70f6c6d into crystal-lang:master Sep 24, 2018
@bcardiff bcardiff added this to the 0.27.0 milestone Sep 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants