1  # Developer guidelines
  2  
  3  - [Developer guidelines](#developer-guidelines)
  4    - [Introduction](#introduction)
  5    - [Glossary](#glossary)
  6    - [Guidelines](#guidelines)
  7      - [Keep the issue tracker clean.](#keep-the-issue-tracker-clean)
  8      - [Keep CI green.](#keep-ci-green)
  9      - [Good issues, good merge requests.](#good-issues-good-merge-requests)
 10        - [Issues/feature requests](#issuesfeature-requests)
 11        - [Merge requests](#merge-requests)
 12      - [Don't push directly to `dev`, use feature branches.](#dont-push-directly-to-dev-use-feature-branches)
 13      - [Review all the things.](#review-all-the-things)
 14      - [Do not merge `WIP` merge requests.](#do-not-merge-wip-merge-requests)
 15      - [Write test.](#write-test)
 16      - [Features development lifecycle.](#features-development-lifecycle)
 17    - [Conclusion](#conclusion)
 18    - [Revisions](#revisions)
 19  
 20  The aim of this document is to describe and document all the development best practices which should be put
 21  in place when developing Gargantext. 
 22  
 23  ## Introduction
 24  
 25  Gargantext is a dynamic project, which evolved from a Python project into a full-stack functional application,
 26  with Haskell in the backend and PureScript in the frontend. As such, the project has seen many developers
 27  come and go, with work sometimes contributed spontaneously by external people (being an open source,
 28  free software). As every open source project, it's necessary to reach a common ground between developers and
 29  share a set of development guidelines to make sure the project is developed _consistently_; by "consistently"
 30  here we mean that there should be some common driving principles which should ultimately result in the
 31  project being developed in a uniform way. In other words, there should be a clear process shared between
 32  all developers about how to:
 33  
 34  - How to code in a collaborative way;
 35  - Propose and implement new features;
 36  - Report problems and write good bug reports;
 37  - Write tests (when, how, why);
 38  - Manage work effectively between branches.
 39  
 40  The rest of the document try to answer all those questions.
 41  
 42  ## Glossary
 43  
 44  - GIT: _Git_ is a distributed version control system
 45  
 46  - MR: _Merge Request_; usually called "Pull Request" in the GitHub work, is an
 47        event that takes place in software development when a contributor/developer
 48        is ready to begin the process of merging new code changes with the main
 49        project repository.
 50  
 51  - CI: _Continuous Integration_; it's a term referring to the process of automatically
 52        building and testing our software on a remote machine. In Gargantext it's integrated
 53        in Gitlab via their _runners_. A "green CI" means that all the CI steps passes correctly,
 54        and typically these steps involve building and testing the process.
 55  
 56  ## Guidelines
 57  
 58  The following is a non-exhaustive list of the development guidelines.
 59  
 60  ### Main working Branches
 61  
 62  3 main branches are used in the distributed version control system (Git) of GarganText:
 63  - _dev_ branch for latest development
 64  - _testing_ branch for testing by beta tester 
 65  - _stable_ branch for production only
 66  
 67  Accordingly _dev_ commits are then merged into _testing_ then into
 68  _stable_ if and only if the steps of this guideline are respected.
 69  
 70  For development, please add the number of the issue in the branch you
 71  are working on. For instance if you work on the issue with number #507,
 72  use the following branch: dev-507-some-keywords-here
 73  
 74  ### Keep the issue tracker clean.
 75  
 76  **Guideline: periodically scan the issue tracker for issues which are no longer relevant or are now
 77  fixed, and close those tickets. If in doubt, ping the original author and ask for information.**
 78  
 79  Periodically, we should try to make an effort in pruning old tickets, as well as closing issues
 80  which are not relevant anymore. Let's remember that an issue tracker is not an append-only log, we are
 81  supposed to make it grow _smaller_, not bigger: our primary goal should be to have the issue tracker be
 82  as empty as possible. This seems obvious but has several advantages:
 83  
 84  - It improves the external perception of the project: a project with hundreds of tickets gives the
 85    impression it's either abandoned or severely broken (i.e. "why has this project so many bugs?");
 86  - It reduces the chances of _duplication_: the more tickets, the more confusion (for newcomers as well),
 87    and it's therefore easier to accidentally open a duplicate ticket (i.e. a ticket referring to an
 88    already-known bug or feature request).
 89  
 90  ### Keep CI green.
 91  
 92  ** Before asking for a Merge Request, make sure your branch has a green CI.**
 93  
 94  **Guideline: We will not merge any MR which doesn't have green CI. 'docs' failures are allowed.**
 95  
 96  Continuous Integration (e.g. CI) is a "public proof" that the project is building and
 97  behaving correctly. Therefore, it's extremely important that we don't merge anything that doesn't
 98  successfully pass CI. If this is slowing down development it's not a CI problem, it simply means
 99  the project is not being kept to sufficiently high standards and we need to take a step back and
100  fix CI in order to be able to fearlessly march forward. Sometimes there are steps in the CI pipeline
101  which are marked as "optional" (like building the `docs`), and for those we tolerate failures.
102  Therefore, an MR in "Passed with warnings" status is acceptable, a MR with a "Failed" CI status is not.
103  
104  ### Good issues, good merge requests.
105  
106  **Guideline: Write issues and merge requests with a clear description that states the intention and/or
107  the rationale. For issues, try to add reproduction steps.**
108  
109  #### Issues/feature requests
110  
111  When opening a new issue, make sure to do the following:
112  
113  - If this is a feature request, describe why this would be a useful addition and which problem it would solve;
114  - If this is a bug report, try to clearly spell out:
115      * What the bug exactly is;
116      * How does the bug manifests itself;
117      * If relevant, useful system information like the version of GHC or the operating system.
118      * If possible, include some reproduction steps (e.g. "click on button X, select Y, etc etc").
119  
120  #### Merge requests
121  
122  When opening a MR, make sure to do the following:
123  
124  - Check your branch has a green CI first please. If you need help or
125    discussion, use the comments in the issue related to your current
126    working branch.
127  - If this is closing an issue, consider using one of the [closing patterns](https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#default-closing-pattern)
128    to automatically close the associated issue when the MR is merged. For example, you can say "Fixes #XXX"
129    (where `XXX` is an issue number) in the MR description;
130  - Add a good description in the MR which summarises the problem which is solving and, if possible, the
131    approach taken to solve the issue: this will help reviewers (see "Review all the things").
132  
133  ### Don't push directly to `dev`, use feature branches.
134  
135  **Guideline: Use feature/issue branches to contribute work, but do NOT push directly to `main` or `dev`.**
136  
137  We should stop pushing directly on `dev` or the other "important" branches: Instead, we should always open
138  a new MR. There are multiple advantages in doing so:
139  
140  - Reverting work on `dev` is easier, because one can simply revert a single merge commit rather than
141    multiple commits;
142  - We reduce the risk of accidentally _undoing_ work; it already happened in that past that pushing on `dev`
143    accidentally deleted some previous work being committed, and that is very painful, especially due to
144    tendency to force push into branches.
145  - The history becomes easier to follow; we can always see which particular MR introduced a particular
146    feature or regression;
147  - New features or bugs won't get missed (see "Review all the things" section).
148  
149  Typically the argument for pushing directly to `dev` is to make things "quicker", for example to test an
150  experimental Gargantext feature. However, we should be in a position to deploy any given branch on the
151  staging server, so that we don't depend on `dev` being the branch we deploy: this reduces the pressure for
152  pushing directly into `dev`.
153  
154  ### Review all the things.
155  
156  **Guideline: Whenever possible, we should ask our colleagues to review our work.**
157  
158  If possible, we should try to review each other's work, because that helps improving the overall quality of
159  the project. Reviewing is an art in itself; we shouldn't start "cherry-picking" on every single MR, because
160  that's not the spirit. The purpose of the code review is the following:
161  
162  - Offering to the colleague a fresh perspective on a problem ("Have you tried X instead of Y"?) which he/she
163    might not have thought about;
164  - Sharing knowledge with the rest of the team (i.e. the fact I'm now the reviewer means I now know about this
165    feature, and it won't catch me off guard next time I pull the latest version of the project);
166  
167  In the spirit of faster development cycles, we shouldn't necessarily veto any MR (unless there are clear bugs),
168  but if there are concerns with the work being committed, we should share our concerns and perhaps log an issue
169  in the issue tracker to discuss further work (e.g. MR !XXX added feature Y in a way it might be problematic,
170  let's keep on eye if this turns out to be a problem.").
171  
172  To start a review, we should spontaneously pick another colleague and assign him/her as a reviewer in the
173  appropriate Gitlab UI.
174  
175  ### Do not merge `WIP` merge requests.
176  
177  **Guideline: if a Merge Request is marked as `WIP`, do not merge it, because it's still a work in progress.**
178  
179  Gitlab gives the chance of marking a MR as a draft by prepending the text "WIP:" next to the MR's title.
180  Therefore, we should honour the original author's will by not merging this MR until CI passed _and_ the
181  `WIP:` prefix has been removed from the title.
182  
183  ### Write test.
184  
185  **Guideline: try to write a test for each new feature or bug added to Gargantext. If that's hard, it
186  means you are not developing with testability in mind.**
187  
188  Testing is arguably the second most important asset of a project (the first being the non-testing code itself),
189  because it protects us from accidentally introducing regressions in the codebase. You shouldn't believe
190  the folklore of "if it compiles, it works", because it's simply not true: you need tests. Writing tests
191  has all sorts of important advantages:
192  
193  - As said, tests protect against accidentally introducing bugs into existing code;
194  - They give other developers a sort of "free documentation", because by reading the tests, they
195    can learn about the expected behaviour of the code being tested;
196  - They force the developer to think about "how do I test this?", which is annoying at first, but
197    then it encourages more modular code, because:
198    * Pure functions are the easiest to test, so that naturally promotes writing as much pure code
199      as possible;
200    * Effectful polymorphic code (i.e. polymorphic over a constrained monad `m`) is the second easiest
201      to test, so that naturally encourages writing lighter monad stacks which can be tested easily, or
202      functions over a monad `m` where `m` can be `IdentityT`/`StateT`/`ReaderT`.
203  
204  ### Features development lifecycle.
205  
206  **Guidelines: each major feature or architectural decision should be discussed with the team.**
207  
208  For each major feature introduced in Gargantext, there should be a process to propose, discuss and approve
209  the feature, for several reasons:
210  
211  - It gives a chance to all developers to chime in on the discussion, bringing in their experience or
212    expertise;
213  - It gives a chance to all developers to understand "what's coming next" without surprised further
214    down the road, with features added without they knowing about them;
215  - It ensures that each new feature or design choice in Gargantext is well understood, principled and
216    with a clear purpose;
217  - It gives us an historical record of precisely _why_ something was done; we can refer to the original
218    ticket by saying "this feature/design was discussed here and approved by the team".
219  
220  Ideally, we could have the following process, divided in 4 phases:
221  
222  - _Triage_: It starts with a developer opening a new issue in the tracker, describing the feature or the
223    design he/she wants to propose. This ticket should be marked with a `triage` label on Gitlab, and it
224    should contain:
225      * Why such feature or design choice makes sense;
226      * Which is the problem it's trying to solve;
227      * What is the impact on the rest of the system.
228  - _Discussion_: Once the ticket has been opened, it's time for other developers to chime in and discuss
229    the proposal;
230  - _Approval_: Once the proposal has been approved, it should be assigned to a developer and it should be
231    marked with an `approved` label on Gitlab. The old `triage` label should be removed;
232  - _Implementation_: Finally, the ticket gets implemented. This concludes the lifecycle.
233  
234  ## Conclusion
235  
236  We have presented a comprehensive overview on the set of best practices we should put in place within
237  Gargantext to make sure the project thrives, keeps growing and succeeds.
238  
239  ## Revisions and certification
240  
241  - To review and agree with these guidelines, read it eventually improve
242    it and commit a change as signature.
243  
244  2023-07-10: Initial version of this document