Skip to content

Add some guidelines for refactoring #22880

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 19, 2023
Merged

Add some guidelines for refactoring #22880

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
62dbfdb
doc
wxiaoguang Feb 12, 2023
2149eeb
Update docs/content/doc/developers/guidelines-refactoring.en-us.md
wxiaoguang Feb 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions docs/content/doc/developers/guidelines-refactoring.en-us.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
---
date: "2023-02-14T00:00:00+00:00"
title: "Guidelines for Refactoring"
slug: "guidelines-refactoring"
weight: 20
toc: false
draft: false
menu:
sidebar:
parent: "developers"
name: "Guidelines for Refactoring"
weight: 20
identifier: "guidelines-refactoring"
---

# Guidelines for Refactoring

**Table of Contents**

{{< toc >}}

## Background

Since the first line of code was written at Feb 12, 2014, Gitea has grown to be a large project.
As a result, the codebase has become larger and larger. The larger the codebase is, the more difficult it is to maintain.
A lot of outdated mechanisms exist, a lot of frameworks are mixed together, some legacy code might cause bugs and block new features.
To make the codebase more maintainable and make Gitea better, developers should keep in mind to use modern mechanisms to refactor the old code.

This document is a collection of guidelines for refactoring the codebase.


## Refactoring Suggestions

* Design more about the future, but not only resolve the current problem.
* Reduce ambiguity, reduce conflicts, improve maintainability.
* Describe the refactoring, for example:
* Why the refactoring is needed.
* How the legacy problems would be solved.
* What's the Pros/Cons of the refactoring.
* Only do necessary changes, keep the old logic as much as possible.
* Introduce some intermediate steps to make the refactoring easier to review, a complete refactoring plan could be done in several PRs.
* If there is any divergence, the TOC(Technical Oversight Committee) should be involved to help to make decisions.
* Add necessary tests to make sure the refactoring is correct.
* Non-bug refactoring is preferred to be done at the beginning of a milestone, it would be easier to find problems before the release.

## Reviewing & Merging Suggestions

* A refactoring PR shouldn't be kept open for a long time (usually 7 days), it should be reviewed as soon as possible.
* A refactoring PR should be merged as soon as possible, it should not be blocked by other PRs.
* If there is no objection from TOC, a refactoring PR could be merged after 7 days with one core member's approval (not the author).
Copy link
Member

@wolfogre wolfogre Feb 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it's not just a suggestion, it also introduces a new rule, as a patch of

gitea/CONTRIBUTING.md

Lines 122 to 125 in 49919c6

Changes to Gitea must be reviewed before they are accepted—no matter who
makes the change, even if they are an owner or a maintainer. We use GitHub's
pull request workflow to do that. And, we also use [LGTM](http://lgtm.co)
to ensure every PR is reviewed by at least 2 maintainers.

So we have to update CONTRIBUTING.md.

I would agree with:

  1. If a PR has been ignored (no comment, no review) for a long time (more than 7 days), and the author or a maintainer believe it won't stand a long wait (like a refactoring PR), he/she can send a last call to TOC or all maintainers.
  2. After another 7 days, if there is still zero approval, that means a polite refusal, then the PR will be closed to avoid wasting more time, so "a last call" has a cost, use it carefully.
  3. However, if there is no objection from TOC, the PR could be merged with one core member's approval (not the author).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TOC decides. Feel free to edit this PR directly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having a call out to TOC would be very useful - I think we've just started doing this in any case.

* Tolerate some dirty/hacky intermediate steps if the final result is good.
* Tolerate some regression bugs if the refactoring is necessary, fix bugs as soon as possible.