flowkey / UIKit-cross-platform

Cross-platform Swift implementation of UIKit, mostly for Android
MIT License
595 stars 40 forks source link

Add codeowners #285

Closed cshg closed 4 years ago

cshg commented 5 years ago

Type of change: Dev workflows

Motivation (current vs expected behavior)

We want to use codeowners to protect critical code. In order to change those parts of the code a review from at least one "owner" is required. We don't want to introduce unnecessary overheaded by defining overall codeowners. At some point in the future (once the team grows) we might add this for additional safety.

Once this PR is merged we will add the following branch protection rule for master:

image

Please check if the PR fulfills these requirements

michaelknoch commented 5 years ago

I would suggest myself as a codeowner for the animation code. But unfortunately all the animation files are in /sources @cshg

Screenshot 2019-04-23 at 10 12 05
cshg commented 5 years ago

@michaelknoch yea, that's a recurring issue that XCode folder structure and file system structure are out of sync :/ Are there some core files we would want to protect, e.g. *+animations.swift files? And we add the other ones which are really needed line by line? I wouldn't do sth like *Animation*, that would be too general and could apply somewhere where it's not intended.

michaelknoch commented 5 years ago

in general i have the feeling that it may be hard in the future to maintain the codeowner file if we define super specific rules. We could also move all animation related files into an animation subdirectory and reference the directory from the codeowners file. This seems to be a bigger effort for now, but maybe it reduces future pain.

what do you think @cshg ?

cshg commented 5 years ago

@michaelknoch I agree, that moving the Animation specific files in a certain directory in the file structure would be the best for this case. If we put in the effort to do that I would suggest to also do the same for the SDL/rendering related files.

ephemer commented 5 years ago

I like the idea of splitting it up but if we do that it should be motivated by project structural clarity and not by codeowners :) but hopefully it’ll be both in one

cshg commented 5 years ago

@ephemer agree, codeowners is just one additional benefit/motivation to introduce that structural clarity. Since we currently rely on the XCode folder structure the need for that just hasn't been as strong. EDIT: IMO our folder namings should diverge as little as possible between XCode and the actual file structure.