Add STYLEGUIDE.md and update some other md files on best practices #7347

onurctirtir · 2023-11-15T09:36:41Z

No description provided.

src/backend/distributed/README.md

codecov · 2023-11-15T09:42:27Z

Codecov Report

Merging #7347 (39cf4bb) into main (3ce731d) will increase coverage by 0.00%.
Report is 1 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7347   +/-   ##
=======================================
  Coverage   89.60%   89.60%           
=======================================
  Files         282      282           
  Lines       60311    60311           
  Branches     7512     7512           
=======================================
+ Hits        54042    54044    +2     
+ Misses       4114     4113    -1     
+ Partials     2155     2154    -1

CONTRIBUTING-development.md

gurkanindibay · 2023-11-22T09:39:07Z

CONTRIBUTING-development.md

+    ..
+  }
+
+* We **start functions with a comment**:


I think we should add comment if there is specific functionality that needs to be expressed in comments. Good naming should not require a comment

Yeah but this is such an important development-convention change that needs to be taken in a team-wide discussion, would you like to initiate this? :)

I agree good naming is does not really require a comment always. But based on my experience with different codebases having a bit of useless comments in a codebase is much better than having to few comments. If we allow functions without comments, then I believe people will often stop adding comments to functions, even for functions that would benefit from it. Also by forcing writing a comment, people will try their best to make it non-useless and reviewers can easier add suggestions if they think it can be improved. So I think keeping this requirement for a comment would be the best option.

gurkanindibay · 2023-11-22T09:41:34Z

CONTRIBUTING-development.md

+
+* Your PR doesn't introduce a typo or something that you can easily fix yourself.
+
+* After all CI jobs pass, code-coverage measurement job (CodeCov as of today) then kicks in. That's why it's important to make the **tests passing** first. At that point, you're expected to check **CodeCov annotations** that can be seen in the **Files Changed** tab and expected to make sure that it doesn't complain about any lines that are not covered. For example, it's ok if CodeCov complains about an `ereport()` call that you put for an "unexpected-but-better-than-crashing" case, but it's not ok if it complains about an uncovered `if` branch that you added.


I'm not sure that if only ereport is the case. Today, I see that EnsurePropagationToCoordinator is not tested however I think it is tested. I see that in some cases I see the output but still codecov complains

Yeah, there are such cases where codecov unnecessarily complains, this requires spending some time to understand how code-coverage measurement is done via code-cov.

CONTRIBUTING-development.md

gurkanindibay · 2023-11-22T09:56:01Z

CONTRIBUTING-development.md

+
+* If you're adding a new file, consider using `src/test/regress/bin/create_test.py` to create the file. Or if you want to manually create it, make sure that your test file creates a schema and that it drops the schema at the end of the test to make sure that it doesn't leak any objects behind. See which lines `src/test/regress/bin/create_test.py` adds to the test file to understand what you need to do.
+
+  For the object that are not bound to a schema, make sure to drop them at the end of the test too, such as databases and roles.


I think our tests should release all the resources it allocated and should not have a dependency other tests. If required, we can copy paste dependency to the new test or we can build an include mechanism if required

I think our tests should release all the resources it allocated

Yeah, this is what line 149 is talking about?

should not have a dependency other tests.

Open to your suggestions for that, that you can add via Add suggestion button :)

Let's collaboratively work on this PR together! :)

gurkanindibay

Thanks to you to step up and define standards.
I added some comments. Could you evaluate those comments

gurkanindibay · 2023-11-22T09:59:54Z

CONTRIBUTING-development.md

+
+But when you're asking for a review, you're asking for someone to review your work and provide feedback. So, when you're asking for a review, you're expected to make sure that:
+
+* Your changes don't perform **unnecessary line addition / deletions / style changes on unrelated files / lines**.


I wish we had a tool like black in python to standardize emtpty spaces and lines

Can we use below formatter?

https://clang.llvm.org/docs/ClangFormat.html

Don't know, do you have any observations about the usages of that tool that you'd like to share?

clang-format doesn't work well with our current codestyle the times I tried it. But maybe it's possible to only use it for newlines.

JelteF · 2023-12-11T07:12:26Z

CONTRIBUTING-development.md

+* We also have the habits of using a **lowerCamelCase** for some variables named from their type or from their function name, as shown in the examples:
+
+  ```c
+  AlterTableCmd *alterTableCommand = NULL;


Global variables use UpperCamelCase, locals variables use lowerCamelCase.

Ah, I now understand that this is some special case for a global where we don't, because the typename is so similar. Let's make it clear that this is a special case by showing an example of a normal global too.

JelteF · 2023-12-11T07:17:32Z

CONTRIBUTING-development.md

+  }
+  ```
+
+* `#includes` needs to be sorted based on below ordering and then alphabetically  and we should not include what we don't need in a file:


Suggested change

* `#includes` needs to be sorted based on below ordering and then alphabetically and we should not include what we don't need in a file:

* `#includes` needs to be sorted based on below ordering and then alphabetically and we should not include what we don't need in a file:

JelteF · 2023-12-11T07:18:51Z

CONTRIBUTING-development.md

@@ -0,0 +1,150 @@
+# Coding style


nit: I think a better name for this file would be: CONTRIBUTING-styleguide.md (feel free to suggest an even better one)

as discussed over chat. Let's go for STYLEGUIDE.md

JelteF · 2023-12-11T07:25:13Z

CONTRIBUTING-development.md

@@ -0,0 +1,150 @@
+# Coding style


I think it's good to prefix this whole file with a disclaimer, saying something like:

The existing codestyle in our codebase is not super consistent. There are multiple reasons for that. One mic reason is because our codebase is relatively old and our standards have changed over time. The second big reason is that our styleguide is different from Postgres its styleguide and some code is copied from postgres source code and slightly modified. The below rules are for new code. If you're changing existing code that uses a different style, use your best judgement to decide if you use the rules here or if you match the existing style.

JelteF · 2023-12-11T07:34:49Z

src/backend/distributed/README.md

@@ -1749,8 +1749,6 @@ The reason for handling dependencies and deparsing in post-process step is that

 Not all table DDL is currently deparsed. In that case, the original command sent by the client is used. That is a shortcoming in our DDL logic that causes user-facing issues and should be addressed. We do not directly construct a separate DDL command for each shard. Instead, we call the `worker_apply_shard_ddl_command(shardid bigint, ddl_command text)` function which parses the DDL command, replaces the table names with shard names in the parse tree according to the shard ID, and then executes the command. That also has some shortcomings, because we cannot support more complex DDL commands in this manner (e.g. adding multiple foreign keys). Ideally, all DDL would be deparsed, and for table DDL the deparsed query string would have shard names, similar to regular queries.

-`markDistributed` is used to indicate whether we add a record to `pg_dist_object` to mark the object as "distributed".


Let's move all changes to this file to a separate PR. Since it's unrelated (they seem like good changes though).

JelteF · 2024-01-10T11:35:07Z

Let's update this and get this merged.

JelteF · 2024-01-15T14:09:58Z

CONTRIBUTING-development.md

+  * If-and-only-if you're **introducing a user facing change / bugfix**, your PR has a line that starts with `DESCRIPTION: <Present simple tense word that starts with a capital letter, e.g., Adds support for / Fixes / Disallows>`.
+  * **Commit messages** are clear enough if the commits are doing logically different things.
+
+# Regression test best practices


I think this section should go into src/test/regress/README.md

JelteF · 2024-01-15T14:17:24Z

CONTRIBUTING-development.md

+  }
+  ```
+
+# Making a pull request ready for reviews


This can go to the normal CONTRIBUTING.md file

JelteF · 2024-01-15T14:19:29Z

CONTRIBUTING-development.md

@@ -0,0 +1,150 @@
+# Coding style
+
+* We almost always use **CamelCase**, when naming functions, variables etc., **not snake_case**.


I think this should contain a section at the start about citus_indent, there's currently a good one on this in CONTRIBUTING.md. Let's move that to the top of STYLEGUIDE.md instead.

addressed all the comments

JelteF · 2024-01-31T09:11:50Z

CONTRIBUTING.md

-### Following our coding conventions
-
-CI pipeline will automatically reject any PRs which do not follow our coding


I think we should at least keep a link to the styleguide here, otherwise people might not find it themselves.

CONTRIBUTING.md

onurctirtir commented Nov 15, 2023

View reviewed changes

src/backend/distributed/README.md Outdated Show resolved Hide resolved

onurctirtir marked this pull request as ready for review November 15, 2023 09:40

c2main reviewed Nov 20, 2023

View reviewed changes

CONTRIBUTING-development.md Outdated Show resolved Hide resolved

onurctirtir mentioned this pull request Nov 20, 2023

Adds additional alter database propagation support #7253

Merged

c2main reviewed Nov 22, 2023

View reviewed changes

CONTRIBUTING-development.md Outdated Show resolved Hide resolved

gurkanindibay reviewed Nov 22, 2023

View reviewed changes

CONTRIBUTING-development.md Outdated Show resolved Hide resolved

gurkanindibay reviewed Nov 22, 2023

View reviewed changes

gurkanindibay self-requested a review November 22, 2023 09:56

gurkanindibay previously requested changes Nov 22, 2023

View reviewed changes

gurkanindibay reviewed Nov 22, 2023

View reviewed changes

JelteF reviewed Dec 11, 2023

View reviewed changes

JelteF reviewed Jan 15, 2024

View reviewed changes

Add STYLEGUIDE.md and update some other md files on best practices

9ce4e1d

onurctirtir force-pushed the contributing-dev branch from 9f2bfc3 to 9ce4e1d Compare January 31, 2024 08:25

onurctirtir requested a review from JelteF January 31, 2024 08:30

onurctirtir changed the title ~~Add CONTRIBUTING-development.md~~ Add STYLEGUIDE.md and update some other md files on best practices Jan 31, 2024

onurctirtir enabled auto-merge (squash) January 31, 2024 08:32

JelteF reviewed Jan 31, 2024

View reviewed changes

add a link

ea70e63

onurctirtir requested a review from JelteF January 31, 2024 09:28

JelteF reviewed Jan 31, 2024

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

onurctirtir added 3 commits January 31, 2024 12:37

improve

c3225a8

Merge branch 'main' into contributing-dev

0161f69

Merge branch 'main' into contributing-dev

39cf4bb

onurctirtir requested a review from JelteF January 31, 2024 09:56

onurctirtir mentioned this pull request Feb 1, 2024

Adds Grant Role support from non-main db #7404

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add STYLEGUIDE.md and update some other md files on best practices #7347

Add STYLEGUIDE.md and update some other md files on best practices #7347

onurctirtir commented Nov 15, 2023 •

edited

codecov bot commented Nov 15, 2023 •

edited

gurkanindibay Nov 22, 2023

onurctirtir Nov 23, 2023

JelteF Dec 11, 2023 •

edited

gurkanindibay Nov 22, 2023

onurctirtir Nov 23, 2023

gurkanindibay Nov 22, 2023

onurctirtir Nov 23, 2023

gurkanindibay left a comment

gurkanindibay Nov 22, 2023

gurkanindibay Nov 22, 2023

onurctirtir Nov 23, 2023

JelteF Dec 11, 2023 •

edited

JelteF Dec 11, 2023

JelteF Dec 11, 2023

JelteF Dec 11, 2023

JelteF Dec 11, 2023

JelteF Jan 15, 2024

JelteF Dec 11, 2023 •

edited

JelteF Dec 11, 2023

onurctirtir Jan 31, 2024

JelteF commented Jan 10, 2024

JelteF Jan 15, 2024

JelteF Jan 15, 2024

JelteF Jan 15, 2024

JelteF Jan 31, 2024


		* Your PR doesn't introduce a typo or something that you can easily fix yourself.

		* After all CI jobs pass, code-coverage measurement job (CodeCov as of today) then kicks in. That's why it's important to make the tests passing first. At that point, you're expected to check CodeCov annotations that can be seen in the Files Changed tab and expected to make sure that it doesn't complain about any lines that are not covered. For example, it's ok if CodeCov complains about an `ereport()` call that you put for an "unexpected-but-better-than-crashing" case, but it's not ok if it complains about an uncovered `if` branch that you added.


		* If you're adding a new file, consider using `src/test/regress/bin/create_test.py` to create the file. Or if you want to manually create it, make sure that your test file creates a schema and that it drops the schema at the end of the test to make sure that it doesn't leak any objects behind. See which lines `src/test/regress/bin/create_test.py` adds to the test file to understand what you need to do.

		For the object that are not bound to a schema, make sure to drop them at the end of the test too, such as databases and roles.


		But when you're asking for a review, you're asking for someone to review your work and provide feedback. So, when you're asking for a review, you're expected to make sure that:

		* Your changes don't perform unnecessary line addition / deletions / style changes on unrelated files / lines.

	* `#includes` needs to be sorted based on below ordering and then alphabetically and we should not include what we don't need in a file:
	* `#includes` needs to be sorted based on below ordering and then alphabetically and we should not include what we don't need in a file:

		@@ -1749,8 +1749,6 @@ The reason for handling dependencies and deparsing in post-process step is that

		Not all table DDL is currently deparsed. In that case, the original command sent by the client is used. That is a shortcoming in our DDL logic that causes user-facing issues and should be addressed. We do not directly construct a separate DDL command for each shard. Instead, we call the `worker_apply_shard_ddl_command(shardid bigint, ddl_command text)` function which parses the DDL command, replaces the table names with shard names in the parse tree according to the shard ID, and then executes the command. That also has some shortcomings, because we cannot support more complex DDL commands in this manner (e.g. adding multiple foreign keys). Ideally, all DDL would be deparsed, and for table DDL the deparsed query string would have shard names, similar to regular queries.

		`markDistributed` is used to indicate whether we add a record to `pg_dist_object` to mark the object as "distributed".

		@@ -0,0 +1,150 @@
		# Coding style

		* We almost always use CamelCase, when naming functions, variables etc., not snake_case.

		### Following our coding conventions

		CI pipeline will automatically reject any PRs which do not follow our coding

Add STYLEGUIDE.md and update some other md files on best practices #7347

Are you sure you want to change the base?

Add STYLEGUIDE.md and update some other md files on best practices #7347

Conversation

onurctirtir commented Nov 15, 2023 • edited

codecov bot commented Nov 15, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JelteF Dec 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gurkanindibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JelteF Dec 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JelteF Dec 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JelteF commented Jan 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onurctirtir commented Nov 15, 2023 •

edited

codecov bot commented Nov 15, 2023 •

edited

JelteF Dec 11, 2023 •

edited

JelteF Dec 11, 2023 •

edited

JelteF Dec 11, 2023 •

edited