Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
vitcpp merged 1 commit intopostgrespro:masterfromdura0ok:get_nth_dot_spoly
Aug 9, 2023

Conversation

dura0ok
Copy link

@esabol@vitcpp please, review it.

@dura0okdura0okforce-pushed theget_nth_dot_spoly branch 2 times, most recently frome8a53a2 to2c14fb4CompareJuly 31, 2023 09:14

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
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

@esabolesabolAug 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 and vitcpp reacted with thumbs up emoji
@dura0okdura0okforce-pushed theget_nth_dot_spoly branch 4 times, most recently froma75e806 tod77d4d8CompareAugust 1, 2023 06:53
@dura0okdura0ok requested review fromvitcpp andesabolAugust 1, 2023 07:23
@dura0ok
Copy link
Author

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

@esabol
Copy link
Contributor

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 thengit 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
Author

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

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

@dura0okdura0okforce-pushed theget_nth_dot_spoly branch 2 times, most recently fromd51cb2e to446f759CompareAugust 4, 2023 20:38
Copy link
Contributor

@esabolesabol 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 reacted with thumbs up emoji
@dura0ok
Copy link
Author

Can we merge it?@vitcpp


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.

@dura0okdura0okforce-pushed theget_nth_dot_spoly branch 2 times, most recently from0642734 to211a4b3CompareAugust 7, 2023 08:52
@dura0okdura0ok requested a review fromvitcppAugust 7, 2023 09:14
@dura0okdura0okforce-pushed theget_nth_dot_spoly branch 2 times, most recently fromf5b83ef todf75911CompareAugust 7, 2023 10:57
@dura0okdura0ok requested a review fromvitcppAugust 7, 2023 18:07
@vitcppvitcpp merged commit77929d7 intopostgrespro:masterAug 9, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@vitcppvitcppvitcpp approved these changes

@esabolesabolesabol 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
[8]ページ先頭
©2009-2025 Movatter.jp