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.

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

Jump to bottom

[ISSUE #16] Implemented a function and tests to extract vertices from spoly by index #37

Merged
merged 1 commit into from
Aug 9, 2023
Merged

[ISSUE #16] Implemented a function and tests to extract vertices from spoly by index #37

merged 1 commit into from
Aug 9, 2023

Conversation

dura0ok
Copy link
Contributor

@dura0ok dura0ok commented Jul 31, 2023

@esabol @vitcpp please, review it.

@dura0ok dura0ok force-pushed the get_nth_dot_spoly branch 2 times, most recently from e8a53a2 to 2c14fb4 Compare July 31, 2023 09:14
esabol
esabol suggested changes Jul 31, 2023
View reviewed changes
sql/poly.sql

SELECT spoint( spoly '{(0,0),(1,0),(1,1)}', 3 );

SELECT spoly_as_array( spoly '{(0,0),(1,0),(1,1)}' );
Copy link
Contributor

Choose a reason for hiding this comment

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

I hate to nitpick whitespace, but I recommend removing one of the two spaces after the SELECTs on lines 599, 601, 603, and 605.

Otherwise, it looks good to me! But if you're adding a function, shouldn't there be an upgrade script and version number bump? Or will that be done in a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, but which version should we put? @vitcpp

Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed before, the master branch is the development branch. Once the API is changed I propose to assign a new version 1.3.0. Upgrade script should be named as pg_sphere--1.2.3--1.3.0.sql.in. The same as I proposed in #22.

Copy link
Contributor

Choose a reason for hiding this comment

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

Concerning the use of blank lines for queries separation. I propose to follow the same coding style. There are no blank lines in sql/path.sql but there are blank lines in sql/poly.sql. I propose to remove these redundant blank lines.

Copy link
Contributor

@esabol esabol Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Except PR #38 already bumps the version to 1.3.0 and adds an upgrade script. I propose merging PR #38 first and then git rebase this PR on the new master to add this functionality to the future 1.3.0.

@dura0ok dura0ok force-pushed the get_nth_dot_spoly branch 4 times, most recently from a75e806 to d77d4d8 Compare August 1, 2023 06:53
esabol
esabol reviewed Aug 1, 2023
View reviewed changes
Makefile Outdated Show resolved Hide resolved
@dura0ok dura0ok requested review from vitcpp and esabol August 1, 2023 07:23
@dura0ok
Copy link
Contributor Author

dura0ok commented Aug 2, 2023

What about this pull request? Is it approved? @esabol

@esabol
Copy link
Contributor

esabol commented Aug 2, 2023

What about this pull request? Is it approved? @esabol

It's perfect other than PR #38 also bumps the version to 1.3.0 and adds an upgrade script. So I recommended merging PR #38 first and then git rebase this PR on the new master. Or you could do it in the opposite order, if you prefer.

Thank you, @stepan-neretin7, for all your work!

@dura0ok
Copy link
Contributor Author

dura0ok commented Aug 2, 2023

Thank you also very much for your patient, detailed review of my code (I'm just learning and it's very valuable to me). Yes, I have added a version update here too and there will be no problems with rebase in any order. Therefore, @vitcpp can merge in any order
It turns out that this pull request is also approved by @esabol?

@esabol
Copy link
Contributor

esabol commented Aug 4, 2023

@stepan-neretin7 : Now that PR #38 has been merged, I think this PR could use a git rebase since the changes to the Makefile are no longer needed and the changes to the upgrade script will likely conflict.

@dura0ok dura0ok force-pushed the get_nth_dot_spoly branch 2 times, most recently from d51cb2e to 446f759 Compare August 4, 2023 20:38
esabol
esabol approved these changes Aug 4, 2023
View reviewed changes
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 to me! Thank you, @stepan-neretin7

@dura0ok dura0ok force-pushed the get_nth_dot_spoly branch from 446f759 to 6c5d333 Compare August 4, 2023 21:31
@dura0ok
Copy link
Contributor Author

dura0ok commented Aug 4, 2023

Can we merge it? @vitcpp

vitcpp
vitcpp requested changes Aug 5, 2023
View reviewed changes
sql/poly.sql

SELECT spoint( spoly '{(0,0),(1,0),(1,1)}', 3 );

SELECT spoly_as_array( spoly '{(0,0),(1,0),(1,1)}' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Concerning the use of blank lines for queries separation. I propose to follow the same coding style. There are no blank lines in sql/path.sql but there are blank lines in sql/poly.sql. I propose to remove these redundant blank lines.

src/path.c Show resolved Hide resolved
src/point.c Outdated Show resolved Hide resolved
src/point.h Outdated Show resolved Hide resolved
src/point.h Outdated Show resolved Hide resolved
src/polygon.h Outdated Show resolved Hide resolved
src/polygon.c Outdated Show resolved Hide resolved
src/polygon.c Outdated Show resolved Hide resolved
src/point.h Outdated Show resolved Hide resolved
src/path.h Outdated Show resolved Hide resolved
@dura0ok dura0ok force-pushed the get_nth_dot_spoly branch from 6c5d333 to 1deaec5 Compare August 6, 2023 03:44
@dura0ok dura0ok requested a review from vitcpp August 6, 2023 03:54
vitcpp
vitcpp requested changes Aug 7, 2023
View reviewed changes
src/point.c Outdated Show resolved Hide resolved
src/point.h Outdated Show resolved Hide resolved
src/polygon.c Outdated Show resolved Hide resolved
src/polygon.c Outdated Show resolved Hide resolved
@dura0ok dura0ok force-pushed the get_nth_dot_spoly branch 2 times, most recently from 0642734 to 211a4b3 Compare August 7, 2023 08:52
@dura0ok dura0ok requested a review from vitcpp August 7, 2023 09:14
@dura0ok dura0ok force-pushed the get_nth_dot_spoly branch 2 times, most recently from f5b83ef to df75911 Compare August 7, 2023 10:57
vitcpp
vitcpp requested changes Aug 7, 2023
View reviewed changes
src/path.h Outdated Show resolved Hide resolved
src/path.c Outdated Show resolved Hide resolved
src/polygon.c Outdated Show resolved Hide resolved
@dura0ok dura0ok force-pushed the get_nth_dot_spoly branch from df75911 to 124cca6 Compare August 7, 2023 18:07
@dura0ok dura0ok requested a review from vitcpp August 7, 2023 18:07
@dura0ok dura0ok force-pushed the get_nth_dot_spoly branch from 124cca6 to d88c059 Compare August 7, 2023 18:41
@vitcpp vitcpp merged commit 77929d7 into postgrespro:master Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@esabol esabol esabol approved these changes

@vitcpp vitcpp vitcpp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dura0ok @esabol @vitcpp