Skip to content
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.

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

Add "pgindent" rule for make to run pgindent #71

Merged
merged 2 commits into from
Nov 9, 2023

Conversation

vitcpp
Copy link
Contributor

@vitcpp vitcpp commented Sep 28, 2023

Implemented pgindent rule to run pgindent on the code. Utilities pgindent and pg_bsd_indent should be in the PATH or specified in the make command line: make PGINDENT=<...> PGBSDINDENT=<...> pgindent

yytype_int16
yytype_int8
yytype_uint8
CPoint
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to sort the list - it looks like it is, except for that last value.

@vitcpp
Copy link
Contributor Author

vitcpp commented Nov 1, 2023

@df7cb I sorted the typedefs and rebased the branch. Thank you!

@esabol
Copy link
Contributor

esabol commented Nov 1, 2023

Where do you get pgbsdindent? My postgresql-15.4/src/tools/pgindent/ directory only compiles the pgindent and pgperltidy executables....

Hmmm, the README in postgresql-15.4/src/tools/pgindent/ says to git clone https://git.postgresql.org/git/pg_bsd_indent.git. That compiles pg_bsd_indent, not pgbsdindent. I'm confused as to the difference. I presume they are compatible, but why is the name of the executable different?

Might be time to add a CONTRIBUTING.md file to the repo to explain this stuff?

@df7cb
Copy link
Contributor

df7cb commented Nov 1, 2023

pg_bsd_indent got imported into the postgresql git, but only in the current master branch. The idea is that different branches might have different indentation rules or something.

https://git.postgresql.org/gitweb/?p=postgresql.git;a=tree;f=src/tools/pg_bsd_indent

(pg_bsd_indent is the correct spelling, pgbsdindent doesn't exist. But pgindent exists, for added confusion...)

@df7cb
Copy link
Contributor

df7cb commented Nov 1, 2023

At the top of the pgindent/README file, there is a link to a blog post that explains how to use it on non-core code:

https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/tools/pgindent/README

http://adpgtech.blogspot.com/2015/05/running-pgindent-on-non-core-code-or.html

Dropping a link to at least the first one would make sense, perhaps as a comment on the Makefile pgindent rule.

@esabol
Copy link
Contributor

esabol commented Nov 2, 2023

Oops, I'm not sure why I thought the executable is named "pgbsdindent". The PR description and the changes to the Makefile clearly show its "pg_bsd_indent". I guess I was just temporarily blind. Anyway, thanks for clarifying, @df7cb.

I guess I'd still like to see some text added to the README.pg_sphere file or comment(s) added to the Makefile or a CONTRIBUTING.md file added to the repo which explains where/how to get the pgindent and pg_bsd_indent executables.

@vitcpp
Copy link
Contributor Author

vitcpp commented Nov 2, 2023

@esabol I think, we have to put some explanations to CONTRIBUTING.md but I propose to do it later. Now CONTRIBUTING.md is too old and not complete. It should be updated. What I propose to do right now is to put some information into the Makefile (above the rule). To run pgindent a postgresql source tree should be installed because it contains sources of pg_bsd_indent and pgindent, or these files should be somehow available in PATH by some other means. The pgindent, pg_bsd_indent binaries should be in the PATH.

I agree with @df7cb to put the link into the Makefile:
https://git.postgresql.org/gitweb/p=postgresql.git;a=blob;f=src/tools/pgindent/README

Now we do not use pgindent on a regular basis but it can be used when claiming some code formatting changes.

@df7cb
Copy link
Contributor

df7cb commented Nov 2, 2023

Fwiw I plan to change the PG17 Debian packages to include pg_bsd_indent and pgindent, if that helps.

https://wiki.postgresql.org/wiki/Apt/FAQ#Development_snapshots

@vitcpp vitcpp force-pushed the make-pgindent branch 2 times, most recently from 116ff1c to ed904af Compare November 7, 2023 17:54
@vitcpp vitcpp requested a review from esabol November 7, 2023 18:13
Makefile Outdated
@@ -223,3 +225,27 @@ endif
dist : clean sparse.c sscan.c
find . -name '*~' -type f -exec rm {} \;
cd .. && tar --transform s/$(SRC_DIR)/pgsphere-$(PGSPHERE_VERSION)/ --exclude CVS --exclude .git -czf pgsphere-$(PGSPHERE_VERSION).tar.gz $(SRC_DIR) && cd -

# To launch pgindent, set the PATH environment variable to the directories
Copy link
Contributor

Choose a reason for hiding this comment

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

I propose changing this line to the following:

To use pgindent, set the PATH environment variable to include the directories

Makefile Outdated
# To launch pgindent, set the PATH environment variable to the directories
# containing the binaries pgindent and pg_bsd_indent. It is important to
# utilize a specific version of pg_bsd_indent, which sources can be found
# in the postgresql>/src/tools/pg_bsd_indent directory, where <postgresql>
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a less-than symbol before "postgresql" on this line, I believe.

@vitcpp
Copy link
Contributor Author

vitcpp commented Nov 8, 2023

Rebased, fixed issues reported in the review

Copy link
Contributor

@esabol esabol left a comment

Choose a reason for hiding this comment

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

Looks good!

@vitcpp vitcpp merged commit c0f824b into postgrespro:master Nov 9, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants