-
-
Notifications
You must be signed in to change notification settings - Fork 64
Update contributing page clarifying tutorial structure rules #331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
As discussed in precice/tutorials#228 and partially implemented in precice/tutorials#459 Also updates minor aspects in the whole page (and introduces an intentional typo).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General rules sound reasonable, but I fear that we are putting the bar for contributing higher and higher. We should also accept contributions that are valuable, but where contributers do not follow all the rules. These tutorials will not meet the highest quality requirements (sorry, no gold badge), but we should still have some process to accept them and to be able to reach the finish line without too much work on our side. Otherwise we will see feedback like this here more often.
For me as an experienced developer it is also becoming increasingly harder to follow all the rules and contribute new cases in a reasonable amount of time.
This is something to study in PreECO. I think we need rules, otherwise we have too many special cases to maintain.
We can always help contributors implement the rules or fix things ourselves if we really want the contribution and have the resources and expertise to do that.
In PreECO, we do establish badges. What to do with the non-verified cases is to be determined. There is always the option to contribute in the community projects and this already documented in this page. Tutorials get in our regression tests and we have to maintain them, so I think we set the rules.
It is a general principle of contributing to open-source projects that contributors need to respond to code reviews. Since we have to maintain everything we merge, we have to set some standards.
The structure was there already and discussed openly since long before these cases got merged. In the PRs that a review was requested by me, I pointed out the inconsistency and I was overruled. More cases followed. These extensions to the rules clarify what the guideline should be, which is easy to implement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes seem fine, but the overall effectiveness of this document is rather low in my opinion. Application experts will need to do too much work to adhere to these guidelines and will mostly be demotivated. This document seems rather like guidelines for us, after we receive a dump of files.
I tend to disagree with these statements. I think moving forward it is important for us to increase the quality of contributions. To keep the work on our side manageable. We are also no longer in a state where we need to grow our tutorials.
I think that this was mainly due to the fact that we did no have all guidelines from the beginning. But things are getting more and more stable. And automation will help soon (also developed in preECO). |
HistoryWe started with barely any structure, which by the time of preCICE v1.6.0 grew into this state: https://github.com/precice/tutorials/tree/precice-v1.6 By today's perspective, I think we can call that state "kind of a mess". The structure we were following (and wanted to see) was documented in this page since December 2020. In April 2021, the structure introduced in v2 and the first preCICE distribution was documented. Since then, barely any (if any at all) new guideline has been introduced. This PR clarifies mainly what the Before we release the first preCICE distribution, there was a fever of pull requests updating all tutorials. Some of them were already in construction since longer. In my view, the driving force and argument at that time was "we need to release very soon, we have already invested a lot of time on it, so let's just merge this now even though it is currently a special case and potentially find a uniform solution later". Now is that later time when we need to fix that inconsistency, and we do that by clarifying the guidelines. Clearly, there was a communication issue at the time, as apparently we had diverging lines of development. What this page should conveyBy triggering this discussion, and by continuing in this direction, it looks like this page still reads as a strict law of the Free State of preCICE that if one breaks they should not even dare think of contributing. This is not the purpose of this page. It just describes what a good starting point would be.
Suggestions are welcomeWhat would you like to see, instead? What (specifically) is good, what is bad, what is ugly? edit: I see that the PR title was by itself triggering. I updated that to better reflect the actual changes. |
As discussed in precice/tutorials#228 and partially implemented in precice/tutorials#459
Also updates minor aspects in the whole page (and introduces an intentional typo).
@uekerman makes sense? Anything else to address?
@BenjaminRodenberg @IshaanDesai does this agree with your understanding?