-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 ditto
and nodoc
without colons
#6362
Remove ditto
and nodoc
without colons
#6362
Conversation
My main concern is this breaking for people silently. Probably the best solution would be to compile-error "nodoc" or "ditto" comments to get people to change. This wasn't broken before though, so I'm not sure we should really "fix" it. |
There is likely not much people outside crystal-lang contributors using this undocumented feature @RX14 . Besides that, in the worst case, |
If the behaviour of the compiler is changed, it should raise. I'm not sure this should to be changed at all (at least not right away). I like changing from |
@j8r "ditto" is documented, and plenty of people use it. |
The reference should better use |
I was searching in the code @RX14 - doesn't know it was here, my bad. Itmight also be in other places. OK, for now we can print a warn about the deprecation of |
What to do to merge - do I need to add a deprecated notice, what's the standard way? |
48a0bdf
to
f442533
Compare
f442533
to
3482d7c
Compare
I think that just like in other parts of Crystal, there should be one way to do things unless there is a benefit to having 2. Just like Crystal has fewer aliases than Ruby, I think making things consistent and making it only work with colons makes it easier to search for, easier to tell that it is "special", and simpler for codebases to have a common way of documenting things, since only one will work. I like the idea of raising if the comment line contains just the word |
Sounds good. As an additional enhancement, I'd like that that a comment just has to start with |
8f7f65b
to
5880802
Compare
Rebased to include the change of #6627 by @Sija, which addresses the @straight-shoota's comment above. |
5880802
to
40b7ec4
Compare
What's remaining for merge, all's good? |
40b7ec4
to
162b06c
Compare
Can this PR be milestoned? This will avoid me to change the |
23ea859
to
8c2735c
Compare
I'd prefer if this were two commits, one changing I think we all agree that using the variants with colons is the preferred way. Judging from the approving reactions, it seems also settled that support for non-colon variants should vanish. It is however unclear if this should be silent, only a warning or a compiler error. And when it should be applied. I don't like the current implementation, which just prints a message to STDOUT. Warnings tend to be ignored anyway. IIRC it was decided once that the compiler should either work or fail, but don't issue warnings. I'd be fine with raising or failing silently. Raising for this seems a bit harsh, so I tend to prefer silently ignoring it. The costs of an error are pretty low: You'd have supposedly undocumented features show up in the API docs with a description Maybe the docs generator could issue a warning and/or the formatter detect and adapt usages. The compiler would stop recognizing non-colon variants two releases later. |
a9228e5
to
d2b8ca3
Compare
@straight-shoota that's fine that the compiler doesn't warn before 1.0 stable, but after the release I hope we'll got deprecation warning before having breaking changes like this (despite not-critical, this is only docs). |
I've separated in two commits to follow the suggestion of @straight-shoota . |
@j8r we document them |
Using something uncommon like colons directly show us that they are not regular comments. |
I like more the colon versions. But it's true that unless we validate/parse |
@asterite I disagree. Using a special syntax highlights that this is a special value with special behaviour. It differentiates these values from regular text. When a method is documented by a single word, it might be confusing whether that is just a description or has some special meaning (is plain Omitting the colons might not break anything, but IMO it doesn't improve either. It might be a matter of habit, but I like the We could possibly consider using a unique format for specifying doc options (see #8353). For example a syntax inspired by JavaDoc tags could work for this. But I think I'd still prefer colon delimited keywords for |
It may be a good idea @straight-shoota . |
Just for reference (I'm not saying it's a good or bad idea), I copied |
@asterite |
Let's keep using and encouraging |
I just keep what I've done with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When warnings are reported it should still work. I think the current state is that ditto
emits a warning but is not treated as :ditto:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine to merge, but I have a (non-essential) suggestion.
4393e22
to
0d8efa7
Compare
0d8efa7
to
d7bc5f5
Compare
d7bc5f5
to
18998b0
Compare
@RX14, remains your approval. |
Thanks @j8r |
FYI. I was checking how this was working and it neither outputs the warnings, nor the warning are build with location information to tell the user were to fix the |
We are used to choose
:nodoc:
with colons butditto
without.However the other variant is also supported,
nodoc
and:ditto:
could also be used.Using colons shows that they are special semantics and not another comments.
Since
# :nodoc:
is always used vs.# nodoc
, the diffs are mainly about changing# ditto
to# :ditto:
.