www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - dub package PR social advice needed

reply evilrat <evilrat666 gmail.com> writes:
Hi there,
A few days back one guy opened PR for my old directx bindings, 
asking to merge patch for 68 files where he adds version(Windows) 
to these files, claiming that he has some problem using the 
package on non-Windows platforms (well, that's expected).

While I don't consider this as a harmful I'm still somewhat 
skeptic about such changes that adds little to no value in my 
opinion.
So instead I advised him to try using dub configurations for that 
task, and this where he start acting weird. Basically he just 
tell he don't care about any advises and then just quit...

Well, ok then. But I still wanted to know what people think about 
such situations and hear advises on how to handle them. Should I 
just reject such stuff in the future, or "take down my proud" and 
merge? (though it's not about my proud tbh)

One can read this little conversation here 
https://github.com/evilrat666/directx-d/pull/10
May 09 2020
next sibling parent Sebastiaan Koppe <mail skoppe.eu> writes:
On Saturday, 9 May 2020 at 11:55:36 UTC, evilrat wrote:

 But I still wanted to know what people think about such 
 situations and hear advises on how to handle them. Should I 
 just reject such stuff in the future, or "take down my proud" 
 and merge? (though it's not about my proud tbh)

 One can read this little conversation here 
 https://github.com/evilrat666/directx-d/pull/10


If you think it should not be merged, then don't merge it. Judge 
the pull request on the value it brings and whether it fits the 
project, not to please people.

Besides, you gave a good reason and a good alternative. I don't 
see any problem here.

Finally, he closed the PR. What is there left to talk about?
May 09 2020
prev sibling next sibling parent Jonathan Marler <johnnymarler gmail.com> writes:
On Saturday, 9 May 2020 at 11:55:36 UTC, evilrat wrote:

 Hi there,
 A few days back one guy opened PR for my old directx bindings, 
 asking to merge patch for 68 files where he adds 
 version(Windows) to these files, claiming that he has some 
 problem using the package on non-Windows platforms (well, 
 that's expected).

 While I don't consider this as a harmful I'm still somewhat 
 skeptic about such changes that adds little to no value in my 
 opinion.
 So instead I advised him to try using dub configurations for 
 that task, and this where he start acting weird. Basically he 
 just tell he don't care about any advises and then just quit...

 Well, ok then. But I still wanted to know what people think 
 about such situations and hear advises on how to handle them. 
 Should I just reject such stuff in the future, or "take down my 
 proud" and merge? (though it's not about my proud tbh)

 One can read this little conversation here 
 https://github.com/evilrat666/directx-d/pull/10


You're responses seemed reasonable to me. That contributor didn't 
seem like he wanted to consider your point of view even though 
you were trying to understand his use case.

As for this particular issue, I wasn't sure why he needed those 
versions. And you pointed out that someone may need those 
bindings even if version is not Windows. The caller can always 
wrap their imports in a version(Windows) block but they wouldn't 
be able to remove that condition if it was added to the modules 
internally. So you'd have to enable version Windows to use them 
which would not work for certain use cases.
May 09 2020
prev sibling next sibling parent reply Guillaume Piolat <firstname.lastname gmail.com> writes:
On Saturday, 9 May 2020 at 11:55:36 UTC, evilrat wrote:

 Hi there,
 A few days back one guy opened PR for my old directx bindings, 
 asking to merge patch for 68 files where he adds 
 version(Windows) to these files, claiming that he has some 
 problem using the package on non-Windows platforms (well, 
 that's expected).

 While I don't consider this as a harmful I'm still somewhat 
 skeptic about such changes that adds little to no value in my 
 opinion.
 So instead I advised him to try using dub configurations for 
 that task, and this where he start acting weird. Basically he 
 just tell he don't care about any advises and then just quit...

 Well, ok then. But I still wanted to know what people think 
 about such situations and hear advises on how to handle them. 
 Should I just reject such stuff in the future, or "take down my 
 proud" and merge? (though it's not about my proud tbh)

 One can read this little conversation here 
 https://github.com/evilrat666/directx-d/pull/10


Hello,

To be honest would have I preferred the version(Windows) solution 
to DUB configurations.

Actually DUB configurations are for software configurations (what 
features are in), not what _platforms_ are in. Some people use 
DUB configurations for examples which is really puzzling, you can 
just use a separate DUB description file for that.

One part of our codebase had the exact problem your contributor 
stumbled upon (dplug:x11) beacuse it brings a link time 
dependencies and, like your contributor, DUB doesn't have 
platform-specific dependencies.

In products we have 10 DUB configurations and cannot multiply 
that number by 2 for platform considerations.

So we went with version(linux) there weren't other choices.


Simply said, people often err on the side of the "too-cautious", 
but the more your merge, the more likely you end up with multiple 
strong contributors (with a ramp-up as each side soften to each 
other).
May 09 2020
parent rikki cattermole <rikki cattermole.co.nz> writes:
There is precedence for this also:

It is now implemented like this in druntime for bindings[0][1].

[0] 
https://github.com/dlang/druntime/blob/master/src/core/sys/windows/aclapi.d#L11
[1] 
https://github.com/dlang/druntime/blob/master/src/core/sys/dragonflybsd/err.d#L11
May 09 2020
prev sibling next sibling parent bachmeier <no spam.net> writes:
On Saturday, 9 May 2020 at 11:55:36 UTC, evilrat wrote:


 Well, ok then. But I still wanted to know what people think 
 about such situations and hear advises on how to handle them. 
 Should I just reject such stuff in the future, or "take down my 
 proud" and merge? (though it's not about my proud tbh)


This is why you released your code under the MIT license and 
posted it on Github, where there's a "Fork" button at the top of 
the project page. They've already written a PR that does what 
they want. You're under no obligation to do anything beyond 
responding to the PR - you're not blocking them in any way.
May 09 2020
prev sibling next sibling parent bauss <jj_1337 live.dk> writes:
On Saturday, 9 May 2020 at 11:55:36 UTC, evilrat wrote:

 Hi there,
 A few days back one guy opened PR for my old directx bindings, 
 asking to merge patch for 68 files where he adds 
 version(Windows) to these files, claiming that he has some 
 problem using the package on non-Windows platforms (well, 
 that's expected).

 While I don't consider this as a harmful I'm still somewhat 
 skeptic about such changes that adds little to no value in my 
 opinion.
 So instead I advised him to try using dub configurations for 
 that task, and this where he start acting weird. Basically he 
 just tell he don't care about any advises and then just quit...

 Well, ok then. But I still wanted to know what people think 
 about such situations and hear advises on how to handle them. 
 Should I just reject such stuff in the future, or "take down my 
 proud" and merge? (though it's not about my proud tbh)

 One can read this little conversation here 
 https://github.com/evilrat666/directx-d/pull/10


I partially agree with the PR.

Packages should work out of the box without additional project 
configuration for platform etc.

Like for vibe.d you don't have to specify any platform specific 
things in your configuration etc.

Platform dependencies should be handled in code and not in 
configuration files.

What if a new package manager came along and it made the 
assumption that things should work out of the box regardless of 
platform?

What if someone wanted to use your package without dub and just 
through dmd? There is no way to tell dmd the specification in an 
easy way.

Of course it's your package and up to you but I see it as a red 
flag when packages require extra configuration to be used on 
specific platforms.
May 10 2020
prev sibling parent evilrat <evilrat666 gmail.com> writes:
Thank you guys. This should be enough.



On Saturday, 9 May 2020 at 12:33:43 UTC, Sebastiaan Koppe wrote:

 If you think it should not be merged, then don't merge it. 
 Judge the pull request on the value it brings and whether it 
 fits the project, not to please people.


Yes that's what I think.


 Finally, he closed the PR. What is there left to talk about?


This situation was just ridiculuos. He started nice, but then 
just throwed a tantrum and then seemingly rage quitted.

Normally I would just yeet him to lead developer and if he yeet 
him back to me I'd just yeet down such PR and be done with it.
Adding to previous paragraph this is why I was kind of thinking 
"WTF just happened? Did he expects I merge this because he is 
trying to make me feel guilty? Well I does not really care that 
much".

But I still bother a bit about if this guy is blaming me for his 
project architecture screw up or is it just me became too 
ignorant.
As a programmer I tend to reason a lot about of things while 
trying to stay above such silly egoistic motives.




On Saturday, 9 May 2020 at 12:41:39 UTC, Jonathan Marler wrote:

 As for this particular issue, I wasn't sure why he needed those 
 versions. And you pointed out that someone may need those 
 bindings even if version is not Windows. The caller can always 
 wrap their imports in a version(Windows) block but they 
 wouldn't be able to remove that condition if it was added to 
 the modules internally.


Yeah that's what I thought. Another reason is that DirectX used 
on other platforms too, including XBOX and mobile phones. None of 
these can be qualified as "Windows".




On Saturday, 9 May 2020 at 13:09:41 UTC, Guillaume Piolat wrote:

 To be honest would have I preferred the version(Windows) 
 solution to DUB configurations.

 Actually DUB configurations are for software configurations 
 (what features are in), not what _platforms_ are in. Some 
 people use DUB configurations for examples which is really 
 puzzling, you can just use a separate DUB description file for 
 that.

 In products we have 10 DUB configurations and cannot multiply 
 that number by 2 for platform considerations.

 So we went with version(linux) there weren't other choices.


I am familiar with that problem, dealing with cartesian product 
of number of configurations is tough.

This looks as a good landmark for architecture seam for me.
So I do think this counts as a software configuration, after all, 
one can think that this rendering backend is optional and the app 
will be completely fine without it.

But can't this be handled a bit more elegant by using subpackages 
with configuration specific to that thing?

I mean he was likely doing some graphics app or basic game, so 
this definitely asks for rendering backend separation, and even 
if there is already few configurations in the main package, doing 
it that way should isolate the problem and prevent explosion of 
configurations.



 Simply said, people often err on the side of the 
 "too-cautious", but the more your merge, the more likely you 
 end up with multiple strong contributors (with a ramp-up as 
 each side soften to each other).


I'm actually feeling ok with accepting contributions, I've just 
seen such situations before in other projects, but back then I 
was able to afford being ignorant thinking "Wow, he's nuts".
Now I have to rethink how to deal with this in the future.




On Sunday, 10 May 2020 at 10:40:05 UTC, bauss wrote:

 I partially agree with the PR.

 Packages should work out of the box without additional project 
 configuration for platform etc.

 What if someone wanted to use your package without dub and just 
 through dmd? There is no way to tell dmd the specification in 
 an easy way.

 Of course it's your package and up to you but I see it as a red 
 flag when packages require extra configuration to be used on 
 specific platforms.


I am not really that familiar with how D community handles this 
specific problem.
However entirely disabling the whole API looks like a fail to me.
I'm not so ignorant to turn down such concerns, but at this 
moment I don't see a better solution for this problem, that 
doesn't require ANY configuration.

Like I said, this isn't just for Windows, but also should work 
with Windows Phone, XBOX and UWP.
None of these are covered with compiler version identifiers.
https://dlang.org/spec/version.html#predefined-versions

Anyway, I'll think what can be done to fix the problem without 
limiting the whole thing just to version(Windows).
May 10 2020