[Issue 13409] std.range.padLeft/Right #3765

JackStouffer · 2015-10-23T14:00:04Z

https://issues.dlang.org/show_bug.cgi?id=13409

~~Added the new function padLeft to std.range. Separated padLeft and padRight into separate PRs.~~

JakobOvrum · 2015-10-26T13:42:40Z

I don't see any purpose in splitting it up into separate PRs. We want to merge both at the same time.

JackStouffer · 2015-10-26T13:57:01Z

In previous PRs, I was told by quickfur and burner to split up functions that do not depend on each other into separate PRs in order to keep the diffs smaller and easier to review.

JakobOvrum · 2015-10-26T14:10:14Z

You can accomplish that by putting them in separate commits.

Changes that need to be merged together should be in the same PR (unless, of course, the patches apply to different repositories). The issue with big PRs is when multiple orthogonal changes are bundled together, delaying merging of one change because of issues with an unrelated change.

JackStouffer · 2015-10-26T14:29:27Z

@JakobOvrum added another commit with padRight.

JakobOvrum · 2015-10-26T14:35:24Z

std/range/package.d

+*/
+auto padRight(R, E)(R r, E e, size_t n) if (
+    isInputRange!R &&
+    (hasLength!R && !is(CommonType!(ElementType!R, E) == void)) ||


The padRight version that doesn't require length seemed to be much of the point of the issue report. Will you work on adding that to this PR?

JackStouffer · 2015-10-27T14:24:17Z

@JakobOvrum changed padRight to use an internal struct so the length isn't needed.

JakobOvrum · 2015-10-27T14:34:35Z

std/range/package.d

+    !isInfinite!R &&
+    !is(CommonType!(ElementType!R, E) == void))
+{
+    static struct Result(R, E)


Result doesn't need to be templated, it already has access to R and E (as it's defined inside the padRight template).

A static struct cannot access the frame of the function.

Accessing template parameters doesn't require access to the frame of the function.

Wouldn't making the struct non-templated also get rid of automatic function attributing?

Since a few compiler versions ago, member functions of structs nested in templates have attribute inference. The unit tests will show.

JakobOvrum · 2015-10-27T14:41:34Z

Looks good! Note that you can test @nogc by statically allocating test arrays in the unit test body (or use only instead of arrays).

JackStouffer · 2015-10-27T14:59:27Z

@JakobOvrum fixed issues and added @nogc tests.

JakobOvrum · 2015-10-27T15:19:04Z

std/range/package.d

+{
+    static struct Result(R, E)
+    {
+        R data;


Nitpick, these data members should be private.

JackStouffer · 2015-10-27T18:03:03Z

Fixed everything this time :)

JakobOvrum · 2015-10-28T00:33:05Z

Discussion here is relevant to this PR. Hopefully there won't be long until a decision is made.

JackStouffer · 2015-10-30T13:58:16Z

@JakobOvrum The forum thread ended, I don't see any thing else blocking this.

JakobOvrum · 2015-10-30T14:21:07Z

@JackStouffer, we don't merge non-trivial PRs with just one reviewer giving the go-ahead. So far it's just me, so another reviewer (or several) will have to look over it before we can merge.

JackStouffer · 2015-10-30T14:27:30Z

@JakobOvrum I'm sorry if I seemed like I was asking you to merge this. What I meant by my comment is to point out that this can be reviewed again now that the forum discussion is over.

JakobOvrum · 2015-10-30T14:29:12Z

@JackStouffer, indeed, thank you for posting it here. Sometimes it's hard for potential reviewers to see if a PR is being blocked by something or not, so posting helps. I'll add the relevant label.

JakobOvrum · 2015-10-31T05:30:50Z

How do these relate to std.string.leftJustifier and std.string.rightJustifier?

JackStouffer · 2015-10-31T17:19:22Z

@JakobOvrum I didn't know about those functions, but here are my observations after looking at their source:

Those functions only seems to work on strings, and it's API is slightly different in that it has a default padding element (space)
Those functions do not provide any of the other range primitives if the underlying range provides them
It's counter intuitive that left justify appends to the right, right justify appends to the left
One thing that I can copy from it for padLeft is not requiring a length member if the input is a forward range. I'll static if and use the current implementation if the input has length, or return an static struct

andralex · 2016-03-22T17:04:18Z

std/range/package.d

+    e = element to pad the range with
+    n = the length to pad to
+
+Return:


andralex · 2016-03-22T17:48:32Z

I approve the addition, but I recommend one more careful pass through the documentation. Reviewers, free to pull after that. Thanks!

JackStouffer · 2016-03-22T19:18:51Z

All comments fixed.

JakobOvrum · 2016-03-22T22:43:12Z

std/range/package.d

@@ -9360,3 +9368,293 @@ if (is(typeof(fun) == void) || isSomeFunction!fun)

    auto r = [1, 2, 3, 4].tee!func1.tee!func2;
 }
+
+/**
+Extends the length of the input range `r` by padding out the start of the


I just want to point out that this sentence also begins with a verb and omits the subject...

Nice, but that's an exception because it's the opener of the dox.

JackStouffer · 2016-03-23T17:19:44Z

The auto-tester errors have nothing to do with this change.

Can anyone give a reason why this shouldn't be merged?

EDIT: and another release goes by without this.

wilzbach · 2016-03-24T12:07:54Z

std/range/package.d

+    n = the length to pad to
+
+Returns:
+    a range containing the elements of the original range with the extra padding


nitpick: new sentence start with a capital letter - A.

JackStouffer · 2016-03-24T17:33:17Z

People can make fun of JavaScript's quadratic, 11 line left-pad all they want, but at least that one isn't sitting in PR where no one can use it.

dlang-bot · 2016-03-24T17:40:10Z

Fix	Bugzilla	Description
✓	13409	std.range.padLeft/Right

quickfur · 2016-03-24T18:12:51Z

I agree. I'm gonna stick my neck out here and merge this. Any complaints, please submit followup PRs. Our current approach of requiring perfection before merging is a great discouragement to new contributors and hampers progress. It's time to change that.

quickfur · 2016-03-24T18:12:58Z

Auto-merge toggled on

JackStouffer · 2016-03-24T20:04:58Z

Thanks!

andralex · 2016-03-24T20:10:11Z

yay

brad-anderson · 2016-03-25T00:05:03Z

If you'll forgive a bit of off topic noise I found the timing of this merge unintentionally hilarious given what is going on in the npm world right now. I guess D users won't be forced to use left-pad.io.

quickfur · 2016-03-25T00:28:30Z

lol... Given the current trend, I'm starting to wonder when people will start turning glue code into packages too, so that you don't even need to write any code to glue various micro-libraries together to do something you could, for example, accomplish in 25 lines of code without any dependencies. Hilarious.

wilzbach · 2016-03-25T07:52:34Z

@JackStouffer you might want to add a follow-up PR that adds a note to the changelog.

@ others: it's quite easy to forget adding stuff to the changelog and with git one can still go through the lists of commits between a release. However - if i may ask - what is your current policy for the changelog?

a) Should a submitter provide the change in his PR (might lead to many unnecessary git conflicts as this file is changed often)
b) Should a submitter provide a follow-up PR to the changelog (easy to forget, might lead to git log clutter)
c) Will someone of the maintainers go through the git log (a lot of work, might be challenging to summarize an addition to a non-familiar part of Phobos)

It might make sense to document such guidelines (see the forum discussion on a better contribution guide)

JackStouffer · 2016-03-25T16:04:15Z

This is not in the changelog because the current one is for 2.071, and this will be going into 2.072. I have to wait until 2.071 is released to add it.

JakobOvrum reviewed Oct 26, 2015
View reviewed changes

JakobOvrum changed the title ~~[Issue 13409] std.range.padLeft/Right (Part 1)~~ [Issue 13409] std.range.padLeft/Right Oct 26, 2015

JackStouffer force-pushed the pad branch from ef55107 to d3825f8 Compare October 27, 2015 14:23

JackStouffer force-pushed the pad branch from d3825f8 to 75f2efa Compare October 27, 2015 14:25

JakobOvrum reviewed Oct 27, 2015
View reviewed changes

JackStouffer force-pushed the pad branch 3 times, most recently from c401ae9 to 7edd8fa Compare October 27, 2015 14:58

JakobOvrum reviewed Oct 27, 2015
View reviewed changes

std/range/package.d

{

static struct Result(R, E)

{

R data;

Copy link

Member

JakobOvrum Oct 27, 2015

Choose a reason for hiding this comment

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

Nitpick, these data members should be private.

JackStouffer force-pushed the pad branch from 7edd8fa to b902965 Compare October 27, 2015 18:02

JakobOvrum added the Review:Needs Review label Oct 30, 2015

andralex reviewed Mar 22, 2016
View reviewed changes

std/range/package.d

e = element to pad the range with

n = the length to pad to

Return:

Copy link

Member

andralex Mar 22, 2016

Choose a reason for hiding this comment

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

"Returns:"

JackStouffer force-pushed the pad branch from 0ba37f8 to f30c484 Compare March 22, 2016 19:17

JackStouffer force-pushed the pad branch 2 times, most recently from 5d2c4c9 to f7b4b3f Compare March 22, 2016 21:10

JakobOvrum reviewed Mar 22, 2016
View reviewed changes

wilzbach reviewed Mar 24, 2016
View reviewed changes

JackStouffer force-pushed the pad branch from f7b4b3f to 9c9f494 Compare March 24, 2016 17:31

Fix Issue 13409: add padLeft and padRight

38c243c

JackStouffer force-pushed the pad branch from 9c9f494 to 38c243c Compare March 24, 2016 17:40

quickfur merged commit 1651c78 into dlang:master Mar 24, 2016

JackStouffer deleted the pad branch March 24, 2016 20:04

Uh oh!

[Issue 13409] std.range.padLeft/Right #3765

[Issue 13409] std.range.padLeft/Right #3765

Conversation

JackStouffer commented Oct 23, 2015

Uh oh!

JakobOvrum commented Oct 26, 2015

Uh oh!

JackStouffer commented Oct 26, 2015

Uh oh!

JakobOvrum commented Oct 26, 2015

Uh oh!

JackStouffer commented Oct 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackStouffer commented Oct 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakobOvrum commented Oct 27, 2015

Uh oh!

JackStouffer commented Oct 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackStouffer commented Oct 27, 2015

Uh oh!

JakobOvrum commented Oct 28, 2015

Uh oh!

JackStouffer commented Oct 30, 2015

Uh oh!

JakobOvrum commented Oct 30, 2015

Uh oh!

JackStouffer commented Oct 30, 2015

Uh oh!

JakobOvrum commented Oct 30, 2015

Uh oh!

JakobOvrum commented Oct 31, 2015

Uh oh!

JackStouffer commented Oct 31, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex commented Mar 22, 2016

Uh oh!

JackStouffer commented Mar 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackStouffer commented Mar 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackStouffer commented Mar 24, 2016

Uh oh!

dlang-bot commented Mar 24, 2016

Uh oh!

quickfur commented Mar 24, 2016

Uh oh!

quickfur commented Mar 24, 2016

Uh oh!

JackStouffer commented Mar 24, 2016

Uh oh!

andralex commented Mar 24, 2016

Uh oh!

brad-anderson commented Mar 25, 2016

Uh oh!

quickfur commented Mar 25, 2016

Uh oh!