search

cadets-ca / ets

MIT License
5 stars 3 forks source link

Create contribution rules #59

Open huguesfcadets opened 4 years ago

huguesfcadets commented 4 years ago

Hi guys,

Thanks for your input.

I'd like that we take this opportunity to set clear working agreements for the project. I really appreciate the input from everyone.

I'd like to start with a few suggestions:

  1. One change per pull request. Maybe not well worded. What the idea is to keep pull request small. So we should try to keep related thing together in a single pull request and leave things that can be part of another pull request related to another issue. @LukeTowers, maybe, with the experience you already have with OSS, you have another view on this, or a better way to word it.
  2. Each pull request with its own issue.
  3. Issues are used to document and discuss a specific issue. If an issue lead to another one, lets create a new issue to discuss it thoroughly. Even if it is to abandon it later on because it's finally not relevant (except if we already know it is not relevant).
  4. I like the reasons behind that we should move to the latest release of iOS. Need to be discussed further just to make sure we know how we will handle iPad in the field. Even that change should go under an issue. 50 or so in the field. At some point we need to advise and make sure somebody take care of updating them. @Davisonp
  5. Code... We must thrive to improve the code at all time (ie “Leave the campground cleaner than the way you found it.”). Need to set some standards, and make them automatically validated with SwiftLint. At first we could start with simple things (function length, class length, ...). SwiftLint come with a set of rules, to long for now... But you see the idea.
  6. Tests... We should look at adding automated test. This can prevent regression and reduce the amount of manual test we need to do.
  7. Not sure it belongs here, but test scenarios could help a great deal too.

Your thoughts?

Together we have great experience and are well equipped to move the project forward.

huguesfcadets commented 4 years ago
  1. Using project boards; a common way to see what's next and planning versions.
LukeTowers commented 4 years ago

My responses below:

  1. I agree with this, although it can be a bit tricky to clearly define what exactly the scope of "one change" is. Fixing dark mode support may involve a lot of different files being modified at once, but conceptually it's all part of "one change". As long as everyone involved agrees to be reasonable when interpreting that guideline it's fine to just say "PRs should focus on fixing or improving one thing at a time".

  2. I don't necessarily agree with this one, there are a lot of times when a PR is simple enough to only require itself to present both the problem and the solution. I would agree that larger changes may necessitate a separate issue to discuss the best approach to resolving them, but generally speaking if someone has identified a problem and also the solution then they shouldn't have to submit an issue and a PR, just the PR should be enough. Again, that's only in cases where it doesn't require a lot of discussion and agreement between collaborators, and where the solution has already been identified / implemented.

  3. Sort of similar thoughts on this one, just have to be reasonable when applying it. If there are a few different issues but they're all related and should probably be fixed at the same time (i.e. various problems with darkmode) then they should be a single issue.

  4. I agree with this, although I'm not familiar with the various style guides for Swift / ObjectiveC. @kirvanp any resources that you're aware of / your own personal style guide? I will say however that coding guidelines should be focused on how code is organized and the general styling rules that should be followed; not on specifics like how many characters / lines a function or class is allowed to be. That sort of clean codebase management is best dealt with by knowledgeable developers when reviewing PRs instead of being applied by some automated toolset. When applying these tools to an existing project it's important to start as small as possible, basically just enforcing the existing conventions of the code base. In this case since the majority was authored by @kirvanp it'd be pretty much his recommendations as to how it's structured / styled. Changes can be discussed and implemented in the future as required, but it's better to have consistency and a minimum amount of automated tools yelling at developers than to have "the perfect set of style rules".

  5. Tests can be useful, although they're difficult to get started with and I'm personally not familiar with the resources / best practices available to the Swift / Objective C communities.

  6. Project boards & milestones can be useful ways of organizing the plan for the project, but they're only as good as the data put inside them. For when a project is just starting out in open source and having multiple contributors I personally prefer something as simple as a text / markdown document in the root of the project documenting the project's overarching goal and a basic outline of future plans for the project. Once the project gets more widely used and there are a lot more issues / PRs to deal with that's when it makes sense to develop a flow and use those "fancier" tools.

See https://github.com/octoberrain/meta/blob/master/maintainers-guide.md & https://github.com/octobercms/october/blob/develop/.github/CONTRIBUTING.md for documentation that we've developed at @octobercms over the years as we refined our processes. Keep in mind that our project has existed for over 6 years now and we've just implemented those documents this past year. Developing the processes that get documented takes time, so I wouldn't be too concerned with detailed documenting of processes that don't yet exist :)

kirvanp commented 4 years ago

1) I'm fine with one change per pull request for general refactoring and feature enhancement, but one situation that's different is when we from time to time increase the minimum iOS target. Doing so will generally produce a flurry of warnings about various APIs being deprecated on the new OS. The impression I've gotten from Hughes is that you want the code to compile without warnings and I generally agree with that. However, there needs to be some flexibility here that either we deal with all the deprecation warnings at once or we accept that there will be an interval of time where we have to ignore some of them until they are all dealt with via several PRs. As Luke pointed out, if an API is deprecated, for example the UIRowActions being replaced with UIContextualAction, it may make sense for whoever understands how to fix it to change all the relevant files at once.

4) iOS updates in the field. The iPads are pretty simple to update. I used to raise the target to the current release all the time for iOS 4 through 10 and the users never had a problem updating. Typically I'd do it during the summer or winter when most of the iPads weren't in use and just tell people to update before their next season started. The only reason I stopped at iOS 10 is because our iPad 4s couldn't go past that! The current crop of iPad 6s will max out on iOS 14 or 15.

5) Code quality. The worst parts of the app are the multi-thousand line files that deal with the iCloud integration, HTML report generation, and the dataModel file and I feel bad for you guys if you ever have to go in there. The iCloud one will lose thousands of lines with the use of newer APIs. The HTML one could probably use a good refractor at some point. As for the data model file, that was my amateur attempt to implement the old Model-View-Controller paradigm where all the computation was done in a model file and the view controller files just update the UI. Unfortunately, as the app became large so too did the data model file. Apple has kind of moved on from Model-View-Controller as we see in the SwiftUI literature where SwiftUI is pretty much wired directly to the underlying data and updates itself so it doesn't really need a controller.

6) Tests. I'd love to see more. Xcode has pretty good support for them but the problem is even so it is probably an hour or two to make each test and if you change the UI you have to change the tests as well. If we were going to have even 2 tests per view it would be nearly 100 tests requiring weeks of work. So ultimately there's a cost-benefit thing there between time spent on tests and time spent on features. Ex-Apple employees say Apple rarely uses automated tests internally and, as we all know, their code can be buggy particularly in September when a major release drops.