Fix issue 10893: added missing or renamed DDoc parameters #1536
Conversation
|
LGTM. Most of the chnages are in |
| @@ -2108,12 +2108,12 @@ assert(SysTime(DateTime(-7, 4, 5, 7, 45, 2)).day == 5); | |||
| Params: | |||
| tz = The $(D TimeZone) to set this $(D SysTime)'s time zone to. | |||
| +/ | |||
| @property void timezone(immutable TimeZone timezone) pure nothrow | |||
| @property void timezone(immutable TimeZone tz) pure nothrow | |||
There was a problem hiding this comment.
Why is this change necessary? I know it makes the internal code a little less confusing (having the function and parameter name not being named the same), but if your ddoc pull warns or errors on this style of code then I think the pull should be fixed up. People might want to write code like this without seeing any warnings.
There was a problem hiding this comment.
Well, I had to change either the comment or the code. In this case (and a few others, in fact) I went with changing the code because I, personally, thought 'tz' was better here than 'timezone'. I thought the doc (and the code, in fact) would be confusing with the parameter having the same name of the function.
There's nothing in the patch that dictates this. It's merely my preference in this case.
There was a problem hiding this comment.
Well, in general, I don't want anything to be renamed just to rename it, so I'd prefer that name changes like this weren't made, but more than that, I really don't want any pulls made against std.datetime right now unless they actually need to be made, because I'm in the middle of breaking it up into separate modules, and pulls like this just make my job harder. This doesn't fix any real bugs in the code, and I'm having to go over and fix all of the documentation in std.datetime right now anyway (thanks to things having moved around). So, I'd much appreciate it if you would remove any std.datetime changes from this pull. If there are any real issues with the documentation after I'm done splitting it up, you can create a pull to fix those then.
There was a problem hiding this comment.
I've reverted the fixes to std.datetime.
| @@ -693,7 +693,7 @@ class Element : Item | |||
| * Constructs an Element from a Tag. | |||
| * | |||
| * Params: | |||
| * tag = the start or empty tag of the element. | |||
| * tag_ = the start or empty tag of the element. | |||
| */ | |||
| this(const(Tag) tag_) | |||
There was a problem hiding this comment.
Ideally, this should just be called tag.
There was a problem hiding this comment.
I don't agree. The class has a member named 'tag' and it's being referenced a couple of times in this method. Forgetting one 'this.' might break the code. Adding 'this.' now is easy, but it hurts maintainability.
There was a problem hiding this comment.
Personally, I think that adding the _ is ugly and that it's better to just use this, and FWIW, IIRC, Andrei agrees based on what he's said in the past about this sort of thing. Now, given that it's in std.xml, which is going to get the axe at some point, I'm not sure I care, but in general, I'm definitely against this sort of naming.
There was a problem hiding this comment.
IMO, naming it tag_ is a form of implementation leak. That said, I don't want to debate naming more than that.
Also, this was the way it was before, so... it's alright if this pull doesn't address this "issue".
No problem.
There was a problem hiding this comment.
Feel free to create a PR with the fixed parameter names. It's a bit too bit of a code change to include in this one.
Fix issue 10893: added missing or renamed DDoc parameters
These were found using the PR #2121 (issue 10236)