Hi everyone,
A problem that recently became obviously painful to me is that too many P.R. goes in in DMD without the matching dlang.org specs change.

I'll give some examples:

- The changes to protection attributes (visibility attributes).
  It was a major change to the language, one that was wanted for a long time, and it was
  done quite gracefully except that it wasn't documented at all.
  So anyone looking at https://dlang.org/spec/attribute.html#ProtectionAttribute will see completely outdated informations, including "Protection does not participate in name lookup." (gasp!).
  An issue was raised ( https://issues.dlang.org/show_bug.cgi?id=16004 ) and a P.R. ( https://github.com/dlang/dlang.org/pull/1325 ) which haven't been merged yet. The changes have been in for 3,5 months and released since 2.5 months.

- extern(C++, {class,struct}) : https://github.com/dlang/dmd/pull/5875 is not documented.
  The same happened for most of the C++ enhancements, and the documentation only recently caught up with the implementation.

- version (PlayStation) & PlayStation4 (https://github.com/dlang/dmd/pull/5850) are not documented as reserved

- All the `@safe` improvements are going in without any documentation change, and Walter said he's going to review @safe specs ( https://github.com/dlang/dmd/pull/5876#issuecomment-226985430 ), however stuff like this are easily forgotten, especially when there is no deadline and it is assigned to someone as busy as Walter.

- (Not yet merged) align(n) with CT-known constant: https://github.com/dlang/dmd/pull/5880

- Last but not least: https://github.com/dlang/dlang.org/pull/1040

It seems pretty clear that we currently have a flakey process regarding spec changes, one that doesn't scale and rely on the core contributors knowing "what to do".

Hence, I would suggest we start enforcing that every change that require a spec change must not be merged before a corresponding spec change is raised.

Basically:
- Raise dlang.org P.R. and mark as being `[Pending]`
- Raise DMD P.R. and link to said dlang.org P.R.
- DMD P.R. is reviewed / merged
- dlang.org P.R. can then be unblocked and be reviewed

Thoughts ?
_______________________________________________
dmd-internals mailing list
dmd-internals@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-internals

All such issues should be files as bugzilla entries and marked with the 'spec' keyword.

On 6/23/2016 4:55 PM, Mathias Lang via dmd-internals wrote:
> Hi everyone,
> A problem that recently became obviously painful to me is that too many P.R.
> goes in in DMD without the matching dlang.org specs change.
>
> I'll give some examples:
>
> - The changes to protection attributes (visibility attributes).
>   It was a major change to the language, one that was wanted for a long time,
> and it was
>   done quite gracefully except that it wasn't documented at all.
>   So anyone looking at https://dlang.org/spec/attribute.html#ProtectionAttribute
> will see completely outdated informations, including "Protection does not
> participate in name lookup." (gasp!).
>   An issue was raised ( https://issues.dlang.org/show_bug.cgi?id=16004 ) and a
> P.R. ( https://github.com/dlang/dlang.org/pull/1325 ) which haven't been merged
> yet. The changes have been in for 3,5 months and released since 2.5 months.
>
> - extern(C++, {class,struct}) : https://github.com/dlang/dmd/pull/5875 is not
> documented.
>   The same happened for most of the C++ enhancements, and the documentation only
> recently caught up with the implementation.
>
> - version (PlayStation) & PlayStation4 (https://github.com/dlang/dmd/pull/5850)
> are not documented as reserved
>
> - All the `@safe` improvements are going in without any documentation change,
> and Walter said he's going to review @safe specs (
> https://github.com/dlang/dmd/pull/5876#issuecomment-226985430 ), however stuff
> like this are easily forgotten, especially when there is no deadline and it is
> assigned to someone as busy as Walter.
>
> - (Not yet merged) align(n) with CT-known constant:
> https://github.com/dlang/dmd/pull/5880
>
> - Last but not least: https://github.com/dlang/dlang.org/pull/1040
>
> It seems pretty clear that we currently have a flakey process regarding spec
> changes, one that doesn't scale and rely on the core contributors knowing "what
> to do".
>
> Hence, I would suggest we start enforcing that every change that require a spec
> change must not be merged before a corresponding spec change is raised.
>
> Basically:
> - Raise dlang.org P.R. and mark as being `[Pending]`
> - Raise DMD P.R. and link to said dlang.org P.R.
> - DMD P.R. is reviewed / merged
> - dlang.org P.R. can then be unblocked and be reviewed
>
> Thoughts ?
> _______________________________________________
> dmd-internals mailing list
> dmd-internals@puremagic.com
> http://lists.puremagic.com/mailman/listinfo/dmd-internals
_______________________________________________
dmd-internals mailing list
dmd-internals@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-internals

I'm fine with filling them as issues as well, but the main point here is that taking care of the spec should not be dealt with after the change, but should be part of the criteria for the P.R. to get in, or it will simply be forgotten.

On 06/24/2016 08:14 AM, Walter Bright via dmd-internals wrote:
> All such issues should be files as bugzilla entries and marked with the 'spec' keyword.
>
> On 6/23/2016 4:55 PM, Mathias Lang via dmd-internals wrote:
>> Hi everyone,
>> A problem that recently became obviously painful to me is that too many P.R.
>> goes in in DMD without the matching dlang.org specs change.
>>
>> I'll give some examples:
>>
>> - The changes to protection attributes (visibility attributes).
>>   It was a major change to the language, one that was wanted for a long time,
>> and it was
>>   done quite gracefully except that it wasn't documented at all.
>>   So anyone looking at https://dlang.org/spec/attribute.html#ProtectionAttribute
>> will see completely outdated informations, including "Protection does not
>> participate in name lookup." (gasp!).
>>   An issue was raised ( https://issues.dlang.org/show_bug.cgi?id=16004 ) and a
>> P.R. ( https://github.com/dlang/dlang.org/pull/1325 ) which haven't been merged
>> yet. The changes have been in for 3,5 months and released since 2.5 months.
>>
>> - extern(C++, {class,struct}) : https://github.com/dlang/dmd/pull/5875 is not
>> documented.
>>   The same happened for most of the C++ enhancements, and the documentation only
>> recently caught up with the implementation.
>>
>> - version (PlayStation) & PlayStation4 (https://github.com/dlang/dmd/pull/5850)
>> are not documented as reserved
>>
>> - All the `@safe` improvements are going in without any documentation change,
>> and Walter said he's going to review @safe specs (
>> https://github.com/dlang/dmd/pull/5876#issuecomment-226985430 ), however stuff
>> like this are easily forgotten, especially when there is no deadline and it is
>> assigned to someone as busy as Walter.
>>
>> - (Not yet merged) align(n) with CT-known constant:
>> https://github.com/dlang/dmd/pull/5880
>>
>> - Last but not least: https://github.com/dlang/dlang.org/pull/1040
>>
>> It seems pretty clear that we currently have a flakey process regarding spec
>> changes, one that doesn't scale and rely on the core contributors knowing "what
>> to do".
>>
>> Hence, I would suggest we start enforcing that every change that require a spec
>> change must not be merged before a corresponding spec change is raised.
>>
>> Basically:
>> - Raise dlang.org P.R. and mark as being `[Pending]`
>> - Raise DMD P.R. and link to said dlang.org P.R.
>> - DMD P.R. is reviewed / merged
>> - dlang.org P.R. can then be unblocked and be reviewed
>>
>> Thoughts ?
>> _______________________________________________
>> dmd-internals mailing list
>> dmd-internals@puremagic.com
>> http://lists.puremagic.com/mailman/listinfo/dmd-internals
> _______________________________________________
> dmd-internals mailing list
> dmd-internals@puremagic.com
> http://lists.puremagic.com/mailman/listinfo/dmd-internals

_______________________________________________
dmd-internals mailing list
dmd-internals@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-internals

On 06/24/2016 09:14 AM, Walter Bright via dmd-internals wrote:
> All such issues should be files as bugzilla entries and marked with the 'spec' keyword.

This is fighting consequence and not a problem. Better approach would be to have a simple checklist for all dmd/phobos mergers to follow:

- Does this PR have tests?
- Does this PR have documentation?
- Does language spec and/or dlang.org articles need to be updated? If
yes, is there a PR for that?
- Does ths PR need changelog entry? If yes, is it there?

If there is a failure on any of these points, PR must not be merged, no exceptions or excused. Not even if it is written by you and desperately needed by Andrei - for a simple reason that being too slow will never do as much damage to the language as making bad quality changes.

Of course you have a great point. The trouble is, for me anyway, it's hard to write the spec incrementally. It also tends to make for a crummy spec. I find it easier and works better to take a more holistic view of spec updates.

For example, I am currently working on the list of issues tagged with 'safe'. Many of them interact with each other. It's easier to work on them more or less in parallel when I have the 'context' of how that part of the code works in my head. The same goes for spec updates.

But I don't want it overlooked, hence the shortcomings should all have bugzilla entries.

On 6/27/2016 2:06 AM, Михаил Страшун via dmd-internals wrote:
> On 06/24/2016 09:14 AM, Walter Bright via dmd-internals wrote:
>> All such issues should be files as bugzilla entries and marked with the
>> 'spec' keyword.
>
> This is fighting consequence and not a problem. Better approach would be
> to have a simple checklist for all dmd/phobos mergers to follow:
>
> - Does this PR have tests?
> - Does this PR have documentation?
> - Does language spec and/or dlang.org articles need to be updated? If
> yes, is there a PR for that?
> - Does ths PR need changelog entry? If yes, is it there?
>
> If there is a failure on any of these points, PR must not be merged, no
> exceptions or excused. Not even if it is written by you and desperately
> needed by Andrei - for a simple reason that being too slow will never do
> as much damage to the language as making bad quality changes.
_______________________________________________
dmd-internals mailing list
dmd-internals@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-internals

As usual Walter, the answer is to not push incremental changes to master but instead to develop them in a branch.

On Mon, Jun 27, 2016 at 8:10 PM, Walter Bright via dmd-internals <dmd-internals@puremagic.com> wrote:
> Of course you have a great point. The trouble is, for me anyway, it's hard to write the spec incrementally. It also tends to make for a crummy spec. I find it easier and works better to take a more holistic view of spec updates.
>
> For example, I am currently working on the list of issues tagged with 'safe'. Many of them interact with each other. It's easier to work on them more or less in parallel when I have the 'context' of how that part of the code works in my head. The same goes for spec updates.
>
> But I don't want it overlooked, hence the shortcomings should all have bugzilla entries.
>
>
> On 6/27/2016 2:06 AM, Михаил Страшун via dmd-internals wrote:
>>
>> On 06/24/2016 09:14 AM, Walter Bright via dmd-internals wrote:
>>>
>>> All such issues should be files as bugzilla entries and marked with the 'spec' keyword.
>>
>>
>> This is fighting consequence and not a problem. Better approach would be to have a simple checklist for all dmd/phobos mergers to follow:
>>
>> - Does this PR have tests?
>> - Does this PR have documentation?
>> - Does language spec and/or dlang.org articles need to be updated? If
>> yes, is there a PR for that?
>> - Does ths PR need changelog entry? If yes, is it there?
>>
>> If there is a failure on any of these points, PR must not be merged, no exceptions or excused. Not even if it is written by you and desperately needed by Andrei - for a simple reason that being too slow will never do as much damage to the language as making bad quality changes.
>
> _______________________________________________
> dmd-internals mailing list
> dmd-internals@puremagic.com
> http://lists.puremagic.com/mailman/listinfo/dmd-internals

_______________________________________________
dmd-internals mailing list
dmd-internals@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-internals

On 06/27/2016 01:10 PM, Walter Bright via dmd-internals wrote:
> Of course you have a great point. The trouble is, for me anyway, it's hard to write the spec incrementally. It also tends to make for a crummy spec. I find it easier and works better to take a more holistic view of spec updates.
> 
> For example, I am currently working on the list of issues tagged with 'safe'. Many of them interact with each other. It's easier to work on them more or less in parallel when I have the 'context' of how that part of the code works in my head. The same goes for spec updates.
> 
> But I don't want it overlooked, hence the shortcomings should all have bugzilla entries.

There are always ways to address it. For example, creating aggregated spec change PR with checklist referring to dmd/phobos implementation PRs and modifying iteratively. Or preparing big chunk of related changes in separate upstream branch as Daniel has suggested.

It is state of mind issue, not a technical one. Once ones mind is set strict to fulfill certain contribution criteria, it won't take a long time to find appropriate pattern to do so. The trick is to suppress inherent programmer laziness and actually start looking for that pattern by straight out declaring compromise/exceptions unacceptable.

This is a typical short-term benefit vs long-term benefit choice scenario. Making small compromises to make ones daily work more convenient and productive may seem reasonable but it usually harms product health in the long term, the worse so the more people work on it.

I am pretty sure whatever issue or inconveniences you may have, community will be able to suggest workflow of comparable convenience but without as high maintenance risks.

On 6/27/2016 5:24 AM, Daniel Murphy wrote:
> As usual Walter, the answer is to not push incremental changes to
> master but instead to develop them in a branch.

Small changes are easier to review. Large ones don't get reviewed at all, or get nitpicks like bikeshedding over spelling, or somebody just holds their breath and pulls it.

Maybe doing things as branches works in other organizations where people can gather in the conference room and go over it together, but I don't think it works for us where we work fairly independently in multiple timezones.
_______________________________________________
dmd-internals mailing list
dmd-internals@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-internals

On 6/27/2016 5:40 AM, Михаил Страшун via dmd-internals wrote:
> There are always ways to address it. For example, creating aggregated
> spec change PR with checklist referring to dmd/phobos implementation PRs
> and modifying iteratively. Or preparing big chunk of related changes in
> separate upstream branch as Daniel has suggested.
>
> It is state of mind issue, not a technical one. Once ones mind is set
> strict to fulfill certain contribution criteria, it won't take a long
> time to find appropriate pattern to do so. The trick is to suppress
> inherent programmer laziness and actually start looking for that pattern
> by straight out declaring compromise/exceptions unacceptable.
>
> This is a typical short-term benefit vs long-term benefit choice
> scenario. Making small compromises to make ones daily work more
> convenient and productive may seem reasonable but it usually harms
> product health in the long term, the worse so the more people work on it.
>
> I am pretty sure whatever issue or inconveniences you may have,
> community will be able to suggest workflow of comparable convenience but
> without as high maintenance risks.

In a way, adding action items to bugzilla is doing the checklist, albeit in a different order. I know I rely heavily on bugzilla, often adding things myself as checklist items.

Also, it's often the case that a design change looks great, but comes unglued when it is tried in implementation. (C++ has been burned a couple times by accepting spec changes that nobody had implemented yet.)

The way C++ progress works is much more process oriented than we are, but they have a lot more people working on it than we do. I think you are quite correct in the value of the process for a larger organization, but we're still a pretty small group of core developers.

Doing things in branches works better for larger organizations. For small ones, like us, doing things in branches means it will get essentially zero testing, because everyone will wait until it is done before even looking at it. We just ran into that issue with the SafeInt thing.

As our organization has grown, we have added process here and there as it became clear it was strongly needed. I'm reluctant to preemptively add process, though, before it is an obvious problem.

tl,dr: Larger organizations need more process, but more process needs a larger organization to support it.
_______________________________________________
dmd-internals mailing list
dmd-internals@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-internals

It works here too.  If you recall the way I did ddmd, I made a giant pull request and added hundreds of commits until it was working.  I then rebased that into hundreds of pull requests and got them merged one by one.  This meant there was both a PR complete enough to actually see if it worked, and PRs small enough to be reviewed individually.

The giant PR was never supposed to be pulled, and it never was.  It
existed so that:
1. I wasn't constantly blocked by PR review
2. A complete picture of the changes was available
3. Changes could be fleshed out and evaluated before landing in master

If working on the spec incrementally is hard, don't do it.  Finish the work in a branch, write the spec, then break the work into small reviewable PRs.


On Tue, Jun 28, 2016 at 5:40 AM, Walter Bright <walter@digitalmars.com> wrote:
>
>
> On 6/27/2016 5:24 AM, Daniel Murphy wrote:
>>
>> As usual Walter, the answer is to not push incremental changes to master but instead to develop them in a branch.
>
>
> Small changes are easier to review. Large ones don't get reviewed at all, or get nitpicks like bikeshedding over spelling, or somebody just holds their breath and pulls it.
>
> Maybe doing things as branches works in other organizations where people can gather in the conference room and go over it together, but I don't think it works for us where we work fairly independently in multiple timezones.
_______________________________________________
dmd-internals mailing list
dmd-internals@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-internals