Skip to content

Comments

fix issue 17085 - Documentation for all traits under SomethingTypeOf missing#5747

Closed
ghost wants to merge 5 commits intomasterfrom
unknown repository
Closed

fix issue 17085 - Documentation for all traits under SomethingTypeOf missing#5747
ghost wants to merge 5 commits intomasterfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Sep 27, 2017

Apparently the "SomethingTypeOf" traits were dedicated to an internal usage (for the 'isSomething') but it's certainly too late to change their visibility. For reference, this is the PR that added these traits.

@ghost ghost requested review from PetarKirov and dnadlinger as code owners September 27, 2017 21:13
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @bbasile! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close Bugzilla Description
17085 Documentation for all traits under SomethingTypeOf missing

@MetaLang
Copy link
Member

I don't think there's a need for this. Phobos has a lot of things that are public but undocumented. It's either for historical reasons or because we want to slowly phase them out; just because it's public it doesn't mean we want to have it be part of the official API.

@ghost
Copy link
Author

ghost commented Sep 29, 2017

Okay, other option:I can set them package with another name, refactor the clients and finally add a deprecated public alias using the current names. My only motivation here is just to get the issue closed. 😉

@wilzbach
Copy link
Contributor

:I can set them package with another name, refactor the clients and finally add a deprecated public alias using the current names.

I would be strongly in favour of this direction.

@jmdavis
Copy link
Member

jmdavis commented Sep 29, 2017

And why bother with deprecating anything? We're talking about undocumented symbols. No one should be using them, and if anyone is, and their code breaks because we make them private, well it's their own fault for using an undocumented symbol.

@ghost
Copy link
Author

ghost commented Sep 29, 2017

@andralex, you merged the PR that added those templates. What do you think, document them or make them package (and if so with deprecation ?) ?

@schveiguy
Copy link
Member

No one should be using them, and if anyone is, and their code breaks because we make them private, well it's their own fault for using an undocumented symbol.

I think it has been made clear many times, we can't do this. Whenever we try this (and I've tried it too), the change gets reversed. If we are going to remove public symbols, we need to go through a deprecation process, regardless of whether they are documented or not.

@ghost ghost requested a review from DmitryOlshansky as a code owner September 29, 2017 20:04
@ghost
Copy link
Author

ghost commented Sep 29, 2017

Okay changed for deprecation, thought there's no message so far. What about "will be removed in the 2.078 release". I think i'll still be there to remove the public aliases in 8 months, unless hit by a train.

@ghost
Copy link
Author

ghost commented Sep 29, 2017

Now that i see how it was used in std.uni: the doc was okay i think

@jmdavis
Copy link
Member

jmdavis commented Sep 29, 2017

I think it has been made clear many times, we can't do this. Whenever we try this (and I've tried it too), the change gets reversed. If we are going to remove public symbols, we need to go through a deprecation process, regardless of whether they are documented or not.

Honestly, I think that it's ridiculous for anyone to be using any undocumented symbols and expect their code to not break, and it's adding unnecessary overhead for us to have to worry about it. I'm strongly in favor of saying tough luck to anyone whose code breaks because they used undocumented symbols.

Okay changed for deprecation, thought there's no message so far. What about "will be removed in the 2.078 release". I think i'll still be there to remove the public aliases in 8 months, unless hit by a train.

Normally, when we deprecate things, we have a two year cycle for it (the second year of which involves the symbol being undocumented in addition to being deprecated), but given that no one should have been using any of these ever outside of std.traits, I really don't care about making the deprecation cycle short if we're going to insist of going the deprecation route.

As for the message, just say say something like "BooleanTypeOf is undocumented and was not supposed to be public. Use isBoolean instead" (in the case of BooleanTypeOf), and then comment it with

// Explicitly undocumented. It will be removed in June 2018. @@@DEPRECATED_2018-06@@@

then it will show up with the other deprecations when I grep for them, and they'll get removed next June.

That being said, I think that it's kind of silly to be deprecating these if they're being used in std.uni. I'd argue that either std.uni should be fixed so that it doesn't use them, or if it really does make sense for std.uni to be using them rather than them just being internal helpers to std.traits, then they should probably just be documented. I think that it's highly questionable to be using package with something like std.traits. IMHO, it's not closely associated enough with anything else in Phobos for it to make sense for it to have any functionality that is used elsewhere in Phobos but is not available for general use. package makes sense for stuff where you have a related set of functionality that's split across a set of modules inside a package, not when the modules are effectively unrelated aside from the fact that they both happen to be in the same library. On the whole, I think that using package with std is an anti-pattern.

Basile Burg added 2 commits September 29, 2017 23:35
This reverts commit 129a703.
This reverts commit 86f6210.
@ghost
Copy link
Author

ghost commented Sep 29, 2017

Well i'm not sure.

@ghost
Copy link
Author

ghost commented Sep 29, 2017

You didn't want to fix it. The amount of discussion such a trivial problem has created is pathetic.

@ghost ghost closed this Sep 29, 2017
@schveiguy
Copy link
Member

Honestly, I think that it's ridiculous for anyone to be using any undocumented symbols and expect their code to not break

#3822
#3950

@schveiguy
Copy link
Member

The amount of discussion such a trivial problem has created is pathetic.

Welcome to software development!

@andralex
Copy link
Member

andralex commented Oct 2, 2017

If a user relies on an undocumented symbol, that points two things: (a) there's a bug in the implementation that made them available; and (b) the user has no business relying on undocumented artifacts.

We can and we will remove undocumented symbols without warning. Doing so fixes bugs that made them inadvertently available. User code shouldn't rely on those as much as it shouldn't rely on a bug in the compiler.

The entire SomethingTypeOf idiom has smell. It is pedestrian, repetitive, and arcane - everything NOT characteristic to good D code. I am sorry I have allowed these symbols in the first place, doubly so for not figuring out they added undocumented public symbols, triply so that so much time has passed since.

We should make these symbols package-level and look for better solutions. A simple factorization that comes to mind is moving the type from the name to a template parameter.

@schveiguy
Copy link
Member

the user has no business relying on undocumented artifacts

If this is the official position, I'm all for it, but note that there are many people that do not agree with that position, such as @CyberShadow and @MartinNowak.

@CyberShadow
Copy link
Member

CyberShadow commented Oct 2, 2017

Sorry, when did I say that?

I think not breaking users' code is something we should try to avoid in general, given reasonable justification. If there is a need to change or remove an undocumented symbol, and doing so doesn't break half of D's ecosystem (something the project tester can tell you), it's certainly something worth considering.

If there is no urgent need, we might as well go through an expedited deprecation cycle (we're already at the "undocument" stage, so just go "deprecate" -> "remove").

@schveiguy
Copy link
Member

@CyberShadow not meaning to put words in your mouth, but I thought I've encountered times where changing undocumented public symbols without a deprecation was something you were against. Apologies if I'm incorrect.

This pull request was closed.
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.

7 participants