Hello everyone!
In an attempt to understand architecture in practice, I’ve dived the code base
In the process of studying, I encountered some negative aspects of the written code, in most cases, functions/methods do not have clearly defined data types, as well as return types.
Despite the fact that variables quite often describe the essence of the stored data, lack of typing significantly complicates the perception of the code and makes it difficult to understand relationships between models.
Intuition tells me that the basic elements that will be used in all APIs should definitely support this functionality, however I’m not sure that anyone allowed to perform a refactor on the code base
After reading the guide
I could not understand whether this is a sufficient problem for creating an RFC, and whether a Pull Request with refactoring will be accepted if the RFC is not created (I’m not even sure that I can create an RFC without the contributor title)
I hope I didn’t give the impression that there is something wrong with the code, the problem is how easy it is for beginners to understand core implementation.
Thanks for reading, I hope I chose the right place for the question!
1 Like
Hi @artyomDehtiar,
Thanks for taking a detailed look and considering improvements!
The code base has plenty of room for improvement. We are gradually introducing typing as we update the code base and welcome more types.
We can categorize changes along two dimensions: backward compatibility and scope. Depending on where the changes fall, we’d ask for an RFC (I’m sorry that it’s unclear. _ Anyone_ can create an RFC; you don’t have to be a Maintainer). I attempted to convey the general idea in the picture below. Please don’t overanalyze the details; it’s just intended to provide intuition, and it is not intended to define criteria in any objective way.
The more localized and non-breaking changes are, the easier it is for us to fully understand them by reviewing a Pull Request (PR). If there are breaking changes that happen in lots of places, then it’ll take more effort for us to understand and verify that things still work as intended. We might ask for an RFC to better understand what the changes are supposed to do, or we might ask for smaller PRs.
Also, do note that there can be changes that happen everywhere that are non-breaking and could be “small” enough to be a single PR. Something like enforcing a single Ruff linter rule could fall into that category. Some typing updates could also meet that criteria.
The more localized the change, the easier for us to understand the PR. Also, if the change is everywhere but is a simple mechanical lint-rule-like change that is easy to inspect, it’s easier to understand in the PR.
For typing changes specifically, we have the convenience of gradually introducing them. A series of “obviously correct” small PRs introducing typing in very localized scopes will be reviewed and understood much more quickly than reviewing a hundred files, even with a simple typing change.
5 Likes
Thank you for reply @tslominski !
I’ll recap what I’ve understood from your answer:
- everyone could make a PR
- RFC required prior to PR only if it’s new functionality or improvement for existing functionality
- refactor doesn’t require RFC if it doesn’t break anything and fully compatible with current linter’s rules
- linter rules are also subjects for PR
- refactor which come in portions that are easier to review e.g: one module at once, is likely to be merged, or will be merged faster
3 Likes
Everyone can make a PR or RFC.
RFCs are never required. But we might ask you for one to understand what you’re proposing, or you may create one if you want before doing a lot of PR work.
2 Likes
@tslominski I’ve just tried to push branch with refactor I’ve made, and git rejected me, so apparently it’s not for everyone
May i request a permission to create/push branches? (for PR)
We work off of forks, not branches.
You’ll need to create a fork of tbp.monty first.
1 Like
