New and existing PRs and issues

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

New and existing PRs and issues

Johannes Weskamm
Hi all,


As a new Pull Request appeared today, i think we should talk about how
to finally handle existing and new PRs.

The new PR can be found here: https://github.com/mapproxy/mapproxy/pull/471

I am interested in your opinions, do we have to vote for this? I
personally think that the issue the author tries to solve may be fixed
by a specific web server configuration (proxy config) and does not need
a change in mapproxy itself, but i may be wrong.


Besides, do we start discussion / voting directly in the PR?

And what about existing PRs, should we check all of them against the
current "rules" ?


Greetings,

Johannes


_______________________________________________
MapProxy-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapproxy-dev
Reply | Threaded
Open this post in threaded view
|

Re: New and existing PRs and issues

Ramūnas Dronga
Hello,
You are right, PR #471 it's just for easier deploys, does not fix anything in mapproxy itself.

Regarding PR in general, I think we can discuss and vote there. Would be easier to understand the whole context.
And right now there is no need to vote for each PR in mapproxy-dev, see [1]



On Tue, 29 Sep 2020 at 10:44, Johannes Weskamm <[hidden email]> wrote:
Hi all,


As a new Pull Request appeared today, i think we should talk about how
to finally handle existing and new PRs.

The new PR can be found here: https://github.com/mapproxy/mapproxy/pull/471

I am interested in your opinions, do we have to vote for this? I
personally think that the issue the author tries to solve may be fixed
by a specific web server configuration (proxy config) and does not need
a change in mapproxy itself, but i may be wrong.


Besides, do we start discussion / voting directly in the PR?

And what about existing PRs, should we check all of them against the
current "rules" ?


Greetings,

Johannes


_______________________________________________
MapProxy-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapproxy-dev


--
Best Regards / Pagarbiai,
Ramūnas
+37060047423

_______________________________________________
MapProxy-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapproxy-dev
Reply | Threaded
Open this post in threaded view
|

Re: New and existing PRs and issues

Oliver Tonnhofer-5
Hi,

On 29. Sep 2020, at 09:44, Johannes Weskamm <[hidden email]> wrote:
As a new Pull Request appeared today, i think we should talk about how
to finally handle existing and new PRs.

The new PR can be found here: https://github.com/mapproxy/mapproxy/pull/471

I am interested in your opinions, do we have to vote for this? I
personally think that the issue the author tries to solve may be fixed
by a specific web server configuration (proxy config) and does not need
a change in mapproxy itself, but i may be wrong.


Yes, there is a working and documented solution for this. 

Ramūnas is right that it does not match the "When is a vote required" items. Maybe "Anything that might be controversial.", as it adds code that is not strictly necessary.  Anyhow, this is a new feature and it does not add any tests. So it's broken by definition and should not be merged as is ;-)
(I just had a quick look at the PR and it's likely that the URLs in the capabilities will be wrong with this patch).

Maybe this is a good starting point for contributor guidelines? Requirements for tests/docs/etc.


Besides, do we start discussion / voting directly in the PR?

Discussion and voting on GitHub is OK, but a vote should be announced here as well. A short pointer to the PR will be sufficient.


And what about existing PRs, should we check all of them against the
current "rules" ?

Most open PRs require a review of the code, but also on the feature itself. Is this feature too specific? Is there another solution to it (like for #471 from above)? Does it solve the issue once and for all, or only for single case. Does it introduce new issues? Is this the "right" way to do it?

I definitely failed to communicate these points in the recent years. But this is one of the reasons I/we are handing over the development to a PSC. 

Back to your question: All PR are up for grabs. If any of you think a PR solves an issue for you, please go ahead! Check the questions above. Check for documentation and tests. If in doubt, ask here or ping us (@mapproxy/psc) on GitHub.

A general triage to check wich PR are still valid is also needed. If any of you can spare an hour, go ahead. (You should all be able to add new labels and close issues/PR).


Regards,
Oliver
_______________________________________________
MapProxy-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapproxy-dev