Update modular-code.md#95
Conversation
Erin, Tim & Keerthi revised various sections. Team - please review!
rrrutledge
left a comment
rrrutledge
left a comment
There was a problem hiding this comment.
Some comments that I think make sense to address, but
also approving as the submitted document seems to be in as-good or better state than the master document.
|
I changed the headline formatting of this file as part of #131. So if that branch get merged, we will likely see a bunch merge conflicts with this branch. |
|
Oh well - this PR is nearly two years old 🤷♂ |
lenucksi
added
the
2-structured
ErinMB
requested review from
cewilliams,
gruetter,
lenucksi and
nyeates
as code owners
|
I've merged the |
|
Great! Will take a look. |
|
None of my discussions are resolved. I already signed off, though. I don't think that my comments should block merge. |
gruetter
left a comment
gruetter
left a comment
There was a problem hiding this comment.
I think a couple of explanations are required. I'll approve, trusting that the shepherd of this PR will consider my suggestions.
| * Requirements might be different for writing modular code. | ||
| * Architectural constraints might impact modularity. | ||
| * Developers who develop monolithic code bases might lack the perspective of how modularity might improve how they work. | ||
| * If there is frequent turnover of team members, modularization may not be a priority. |
There was a problem hiding this comment.
Just to clarify: modularization might not be a priority for developers, if they think they are going to leave the team, soon? I would argue it's definitively a priority from an organisations perspective, because modular code should be more understandable and maintainable and that in turn helps when new team members are onboarded.
| * Architectural constraints might impact modularity. | ||
| * Developers who develop monolithic code bases might lack the perspective of how modularity might improve how they work. | ||
| * If there is frequent turnover of team members, modularization may not be a priority. | ||
| * Level of communication between teams can impact ability/inclination to modularize. |
There was a problem hiding this comment.
I believe this also needs some clarification.
spier
left a comment
spier
left a comment
There was a problem hiding this comment.
Given that this PR is open for 2.5 years already, I suspect that it might not have a shepherd any more. 😢
Therefore my suggestion would be for anybody of any Trusted Committer here that is still part of the conversation to resolve the comments to the best of their knowledge, and then merge the PR.
That way we capture at least some of the value that this PR, without keeping it open for an unnecessarily long amount of time.
Once the PR is merged, others can take this pattern forward, as it is currently still in "Initial" state i.e. a pretty immature pattern anyways.
Co-authored-by: Sebastian Spier <github@spier.hu>
Co-authored-by: Sebastian Spier <github@spier.hu>
Co-authored-by: Sebastian Spier <github@spier.hu>
lenucksi
added
📖 Type - Content Work
lenucksi
left a comment
lenucksi
left a comment
There was a problem hiding this comment.
This looks fine to me for its current placement in 1-initial.
Looking for a reaction to @gruetter's review comments in #95 (review) by @NewMexicoKid and @MaineC and will then merge once those are resolved.
Co-authored-by: Sebastian Spier <github@spier.hu>
|
I worked in most of the feedback from this PR, and left the threads open that I could not resolve myself. Even though there are open questions from @gruetter I am merging this PR now, as it already contains a number of improvements over the pattern currently in the mainline. |
7 participants
Erin, Tim & Keerthi revised various sections. Team - please review!