Skip to content

fix: change target_srs argument to target_crs#108

Merged
JinIgarashi merged 9 commits intomainfrom
fix/stats-package
Jan 10, 2025
Merged

fix: change target_srs argument to target_crs#108
JinIgarashi merged 9 commits intomainfrom
fix/stats-package

Conversation

@Thuhaa
Copy link
Contributor

@Thuhaa Thuhaa commented Jan 8, 2025

Instead of passing target_srid as a integer, It is better to use target_srs as an argument to give user the freedom to use whichever they want

@Thuhaa Thuhaa temporarily deployed to azure container registry January 8, 2025 08:04 — with GitHub Actions Inactive
@Thuhaa Thuhaa temporarily deployed to github container registry January 8, 2025 08:04 — with GitHub Actions Inactive
JinIgarashi
JinIgarashi reviewed Jan 8, 2025
View reviewed changes
Copy link
Contributor

@JinIgarashi JinIgarashi left a comment

Choose a reason for hiding this comment

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

@Thuhaa looks good. please fix your changes to pass tests

JinIgarashi
JinIgarashi suggested changes Jan 9, 2025
View reviewed changes
Copy link
Contributor

@JinIgarashi JinIgarashi left a comment

Choose a reason for hiding this comment

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

@Thuhaa I realized current api can handle both absolute and relative path. you don't need to change anything about file path.

please revert all changes related to file path you made. and make this PR purely SRID changes.

@Thuhaa Thuhaa temporarily deployed to azure container registry January 10, 2025 09:01 — with GitHub Actions Inactive
@Thuhaa Thuhaa temporarily deployed to github container registry January 10, 2025 09:01 — with GitHub Actions Inactive
@Thuhaa Thuhaa temporarily deployed to azure container registry January 10, 2025 09:08 — with GitHub Actions Inactive
@Thuhaa Thuhaa temporarily deployed to github container registry January 10, 2025 09:08 — with GitHub Actions Inactive
@Thuhaa Thuhaa requested a review from JinIgarashi January 10, 2025 09:14
JinIgarashi
JinIgarashi suggested changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@JinIgarashi JinIgarashi left a comment

Choose a reason for hiding this comment

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

@Thuhaa could you change target_srs to target_crs? otherwise, it looks good

@Thuhaa Thuhaa temporarily deployed to github container registry January 10, 2025 09:50 — with GitHub Actions Inactive
@Thuhaa Thuhaa temporarily deployed to azure container registry January 10, 2025 09:50 — with GitHub Actions Inactive
@Thuhaa Thuhaa changed the title fix: path names in stats package and change target_srid to target_srs fix: change target_srs argument to targe_crs Jan 10, 2025
@Thuhaa Thuhaa changed the title fix: change target_srs argument to targe_crs fix: change target_srs argument to target_crs Jan 10, 2025
JinIgarashi
JinIgarashi suggested changes Jan 10, 2025
View reviewed changes
cbsurge/stats/OGRDataSource.py Outdated
Comment on lines 93 to 94
target_crs (str): EPSG code or PROJ.4 string for the target projection.
src_srs (str): EPSG code or PROJ.4 string for the source projection (optional).
Copy link
Contributor

@JinIgarashi JinIgarashi Jan 10, 2025

Choose a reason for hiding this comment

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

please update parameter description. change src_srs to src_crs

Copy link
Contributor Author

@Thuhaa Thuhaa Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Parameter description has been updated from src_srs to src_crs

Copy link
Contributor

@JinIgarashi JinIgarashi Jan 10, 2025

Choose a reason for hiding this comment

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

you did not update help string for parameter

Copy link
Contributor

@JinIgarashi JinIgarashi Jan 10, 2025

Choose a reason for hiding this comment

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

@Thuhaa I think PROJ.4 string is no longer expected to be passed by users?

Copy link
Contributor Author

@Thuhaa Thuhaa Jan 10, 2025

Choose a reason for hiding this comment

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

I have fixed

Copy link
Contributor

@JinIgarashi JinIgarashi Jan 10, 2025

Choose a reason for hiding this comment

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

@Thuhaa you did not fix description

@Thuhaa Thuhaa temporarily deployed to github container registry January 10, 2025 10:59 — with GitHub Actions Inactive
@Thuhaa Thuhaa temporarily deployed to azure container registry January 10, 2025 10:59 — with GitHub Actions Inactive
@Thuhaa Thuhaa temporarily deployed to azure container registry January 10, 2025 11:04 — with GitHub Actions Inactive
@Thuhaa Thuhaa temporarily deployed to github container registry January 10, 2025 11:04 — with GitHub Actions Inactive
@Thuhaa
Copy link
Contributor Author

Thuhaa commented Jan 10, 2025

@Thuhaa I realized current api can handle both absolute and relative path. you don't need to change anything about file path.

please revert all changes related to file path you made. and make this PR purely SRID changes.

@JinIgarashi Changes for the file paths have been reverted

@Thuhaa Thuhaa temporarily deployed to github container registry January 10, 2025 11:26 — with GitHub Actions Inactive
@Thuhaa Thuhaa temporarily deployed to azure container registry January 10, 2025 11:26 — with GitHub Actions Inactive
JinIgarashi
JinIgarashi suggested changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@JinIgarashi JinIgarashi left a comment

Choose a reason for hiding this comment

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

@Thuhaa you missed to change here. please check all your changes again to make sure everything is fine.

@Thuhaa Thuhaa temporarily deployed to github container registry January 10, 2025 11:38 — with GitHub Actions Inactive
@Thuhaa Thuhaa temporarily deployed to azure container registry January 10, 2025 11:38 — with GitHub Actions Inactive
@Thuhaa Thuhaa temporarily deployed to github container registry January 10, 2025 11:39 — with GitHub Actions Inactive
@Thuhaa Thuhaa temporarily deployed to azure container registry January 10, 2025 11:39 — with GitHub Actions Inactive
@Thuhaa Thuhaa requested a review from JinIgarashi January 10, 2025 11:58
@JinIgarashi JinIgarashi merged commit bccb1c9 into main Jan 10, 2025
3 checks passed
@JinIgarashi JinIgarashi deleted the fix/stats-package branch January 10, 2025 12:13
iferencik pushed a commit that referenced this pull request Apr 10, 2025
@Thuhaa
fix: change target_srs argument to target_crs (#108)
f8dec2b 
* fix: path names in stats package and change target_srid to target_srs

* refactor: revert path changes and fix issue with srcDS arg of vector translate

* fix: tests

* refactor: change target_srs argument to target_crs

* fix: update crs comments

* fix: update help message for -target-crs argument

* fix: update comments

* fix comments

* fix comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@JinIgarashi JinIgarashi JinIgarashi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Thuhaa @JinIgarashi