Balance project values

[kdmnk]

In the recent period, I mostly took the role of a "code quality officer" rather than a simple reviewer.
I have a hard time balancing what should be allowed to go into our project and what not.

In our contributing guidelines I clearly stated that we prioritise readability. As you all know, the project is way bigger that we originally anticipated (and there are still many modules that we can add). We have been working since more than 2 years now to refactor and to standardise our code base. In this sense, I have requested changes on recent PRs that did not fit my definition of readable code (which also usually includes common practices and patterns in general, not allowing error prone quick fixes). I also believe that these requested changes contribute to a learning experience that is hard to experience otherwise.

However, pushing changes to potentially help new beginner developers with clean code should not let the current developers to feel pressed on quality and other constraints. Everyone should be able to write code that makes them motivated and experience success.
We already had some exceptions of merging code that has to be improved later, which is fine by me in exceptional cases but otherwise my goal was to handle the issues in early stages, even if the scope got bigger than a "small bugfix". I understand that that might be annoying.

When I proposed those bigger changes, I also don't know what is the good solution, whether the implementation worth the effort at all, I'm just offering some paths that we can explore and learn about.

After all, this project is still a playground, at least to me, and this gives us the possibility to learn and explore new things, to spend hours on something really nice and cool just to realise it did not worth it, cause that's how we learn how and when to use the same architecture and patterns later.

I wanted to hear how do you feel about this, and suggest how should we proceed. I am still busy solving our old problems, I just don't want (or rather simply, I won't) start fixing new problems introduced. Maybe it's fine like that..?
Slightly connected, but also please let me know if you have any insights how the integration of beginners went.

[horcsinbalint]

First of all, I am extremely happy that you and others are passionate about this project, and thanks for starting this discussion.

To the contrary, I am mostly interested in operational parts of how IT services are managed in the College. While I would love to also do web development for Mars as it can be quite rewarding, I have projects for both the College and other organizational units that I like to contribute to more as they provide more challenges.

Therefore, I do not have plans on refactoring code that is functional (I am usually in favor of not touching something if the thing is functioning properly) or to add larger features. And I also usually don't have time checking if a change has introduced new bugs and I was worried about the changes that were deployed in #551. It seemed to me that it might not have been properly tested, and that is why I opposed the changes. And I think that my concerns had some merit (there are several issues with applications now).

I think the only PR that I could debate why it was not merged in a timely fashion is #562. For other PR's I think the decision of not merging was balanced.

So I prefer stability and security over newer features or readability. So I am essentially debating what values we should be looking in the project. On the other hand, code quality should usually be improved without functional regression, so these values can co-exist, and better quality code should have fewer bugs.

So I think, for new features and refactoring, a stricter approach could be used. For bugfixes, the benefits and downsides of making a quick fix and opening a seperate issue should be weighed against making the whole fix in one PR.
Cases where a quick fix's benefits clearly outweighed their downsides in my opinion:

• most security related bugs
• most cases where the reputation of the College was at stake (serving an internal server error to students who are planning to apply to the College is quite bad - the fixes were introduced without a PR in 61d5bb9 but I have reviewed them before deployment)

But I would prefer bugfixes and refactoring compared to adding newer features. And would probably consider removing some (languages other than English and Hungarian, WiFi connections table - it does not receive the required data as it would make the production web server a more valuable target as radius logs contain live data that can be used to geolocate users).

[horcsinbalint]

These were just my two cents on the question and I would not want to interfere with how the code is created in general as I am not that really experienced in this project - that was also a reason why I turned down to be the contact between users, staff and developers. So you may continue as you see fit.

For me, I have also have sometimes problems with code quality but can find answers to my questions quite easily, but I do not have a clear way of making it approachable for beginners.

Slightly connected, but also please let me know if you have any insights how the integration of beginners went.

I do not exactly know as the metrics of the project did not change drastically, I have not yet received the end-of-semester reports on what others thought of it, and I have not attended some of the lessons.