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.db.connect: Replace -o flag with the standard --overwrite #3214

Merged
merged 2 commits into from
Mar 23, 2024

Conversation

HuidaeCho
Copy link
Member

This PR fixes #3149 by using the standard --overwrite flag.

@HuidaeCho HuidaeCho added database Related to database management vector Related to vector data processing bug Something isn't working labels Oct 22, 2023
@HuidaeCho HuidaeCho changed the title v.db.connect: Implement standard --overwrite and remove -o flag v.db.connect: Replace -o flag with the standard --overwrite Oct 22, 2023
@HuidaeCho HuidaeCho requested a review from cmbarton October 22, 2023 12:43
@marisn
Copy link
Contributor

marisn commented Oct 22, 2023

I'm sorry, but this shall not pass. You should not remove a flag in non-major release.
Please add a printf with warning whenever -o is used: "The -o flag is deprecated and will be removed in future releases. Use --o instead"
To future proof the code, wrap the old -o in a macro #if GRASS_VERSION_MAJOR < 9

@neteler neteler added this to the 8.4.0 milestone Nov 4, 2023
Copy link
Member

@landam landam left a comment

Choose a reason for hiding this comment

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

@HuidaeCho It make sense to me to add --overwrite flag. On the other hand we cannot break backward compatibility in GRASS 8 releases. The -o flag could be removed in GRASS 9. I suggest to add depreciation warning that -o will be removed in GRASS 9.

@HuidaeCho
Copy link
Member Author

This PR requires #3256.

@HuidaeCho HuidaeCho marked this pull request as ready for review November 22, 2023 04:26
@HuidaeCho
Copy link
Member Author

@marisn, @landam Will this work with #3256?

@marisn
Copy link
Contributor

marisn commented Dec 3, 2023

@marisn, @landam Will this work with #3256?

Yes. I tested together with #3256 and backwards compatibility is in place.

@HuidaeCho HuidaeCho requested a review from landam December 7, 2023 15:42
@HuidaeCho HuidaeCho force-pushed the v_db_connect_overwrite branch from 67a285d to ab14fac Compare January 9, 2024 17:48
@github-actions github-actions bot added C Related code is in C libraries module and removed database Related to database management labels Jan 9, 2024
@HuidaeCho HuidaeCho force-pushed the v_db_connect_overwrite branch from ab14fac to 5bbcb15 Compare February 16, 2024 02:52
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.

Maybe a test to prove that both -o and --overwrite work? Probably using gs.run_command(...) (with flags="o" and overwrite=True) rather than the assert*() functions which rely on grass.pygrass Module interface which performs checks on its own and does not support lib/gis/renamed_options.

Copy link
Member

@landam landam left a comment

Choose a reason for hiding this comment

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

I successfully tested:

v.db.connect map=voda_aoi table=VodniTok
ERROR: Use --overwrite to overwrite existing link for layer <1>

v.db.connect map=voda_aoi table=VodniTok --o
The table <VodniTok> is now part of vector map <voda_aoi> and may be
deleted or overwritten by GRASS modules

v.db.connect map=voda_aoi table=VodniTok --o
The table <VodniTok> is now part of vector map <voda_aoi> and may be
deleted or overwritten by GRASS modules

@landam
Copy link
Member

landam commented Mar 22, 2024

@HuidaeCho Please feel free to merge (you have more than zero approvals)...

@HuidaeCho HuidaeCho force-pushed the v_db_connect_overwrite branch from 69ab709 to 1af485a Compare March 23, 2024 00:10
@HuidaeCho HuidaeCho merged commit 2c298d9 into OSGeo:main Mar 23, 2024
26 checks passed
@HuidaeCho HuidaeCho deleted the v_db_connect_overwrite branch March 23, 2024 04:30
@wenzeslaus wenzeslaus removed the request for review from cmbarton April 23, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working C Related code is in C libraries module vector Related to vector data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] v.db.connect does not recognize --overwrite flag
5 participants