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

v.random: finished test in reference to #704 #1464

Merged
merged 8 commits into from Mar 24, 2021

Conversation

SunveerSingh
Copy link
Contributor

As said in the pull: #704

JosefPudil and others added 2 commits June 10, 2020 13:41
1) Checking if number of points equals 100
2) Checking if all points are in the polygon boundary state
3) Checking if all points are in the polygon boundary state
As said in the pull: OSGeo#704
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Thank you for opening a new PR. The history is preserved which is great!

I have some new comments and some same as in #704.

vector/v.random/testsuite/test_v_random.py Outdated Show resolved Hide resolved
vector/v.random/testsuite/test_v_random.py Outdated Show resolved Hide resolved
vector/v.random/testsuite/test_v_random.py Show resolved Hide resolved
vector/v.random/testsuite/test_v_random.py Outdated Show resolved Hide resolved
vector/v.random/testsuite/test_v_random.py Outdated Show resolved Hide resolved
@SunveerSingh
Copy link
Contributor Author

@wenzeslaus Please see the updated commit e7429eb

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Actually, there is still one issue remaining from #704. While at it, add also a author/license comment.

Additionally, please update title and description of this PR to reflect what is being added to GRASS GIS so that these texts can become a commit message. (I will update them as needed before merging, but do your best in expressing that, read something online, see some simple rules here, for example, updating would be misleading when no existing file in GRASS GIS is being updated.) Besides the benefit for the commit history, for PR itself (and any of your future PRs), this make it clear what is being proposed and it makes it easier for people to evaluate if they should look at this and what to review.

vector/v.random/testsuite/test_v_random.py Outdated Show resolved Hide resolved
vector/v.random/testsuite/test_v_random.py Outdated Show resolved Hide resolved
@SunveerSingh SunveerSingh changed the title Update v.random test v.random: finished test in reference to https://github.com/OSGeo/grass/pull/704 Mar 24, 2021
@SunveerSingh SunveerSingh changed the title v.random: finished test in reference to https://github.com/OSGeo/grass/pull/704 v.random: finished test in reference to #704 Mar 24, 2021
Comment on lines 59 to 61
"""Checking if all points are in the polygon boundary state"""

def test_restrict(self):
Copy link
Member

Choose a reason for hiding this comment

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

Change of syntax is not enough here. This is not documenting the function. See how docstrings for functions work, e.g., here. The result will then make more sense in the terms of formatting (empty lines, Black). Run the code but locally make some modification so that the test fails. You will the see the docstring reported with the failed tests as I mentioned earlier.

@SunveerSingh
Copy link
Contributor Author

@wenzeslaus Please see the updated commit. be3120d

@wenzeslaus wenzeslaus merged commit 0f0b7f8 into OSGeo:master Mar 24, 2021
@wenzeslaus
Copy link
Member

Thank you. Merged! For future reference, see the commit message (title/subject/first line and body/description) I used.

@SunveerSingh
Copy link
Contributor Author

Thanks a lot!

@neteler neteler added this to the 8.0.0 milestone Dec 9, 2021
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022
Check number of points, z, polygon boundary.

Initially version submitted as OSGeo#704.

Co-authored-by: JosefPudil <pepa.pudil@seznam.cz>
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
Check number of points, z, polygon boundary.

Initially version submitted as OSGeo#704.

Co-authored-by: JosefPudil <pepa.pudil@seznam.cz>
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.

None yet

4 participants