-
Notifications
You must be signed in to change notification settings - Fork 492
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
Add .qpj and .qmd Extensions to Shapefile Handling #8134 #10305
Conversation
@Saixel this is a great start. Next, can you please review this page and consider if any changes should be made? https://dataverse-guide--10305.org.readthedocs.build/en/10305/developers/geospatial.html . As I may have mentioned, I'm not especially familiar with this feature so we'll need to learn together about it. 😅 The source is doc/sphinx-guides/source/developers/geospatial.rst Also, once we're sure of the direction we're taking, we'll want to add a release note snippet. Please read this: https://preview.guides.gdcc.io/en/develop/developers/version-control.html#writing-a-release-note-snippet Unfortunately, I don't see any tests being run by Jenkins (this is a recent problem where the webhook isn't delivered). Maybe if we push another commit, they will start. We'll want to check that the tests are passing. Speaking of tests, can you add any assertions of the new behavior to existing tests? Finally, please add this PR to https://github.com/orgs/IQSS/projects/34 . For now the "in progress" column is a good place. You can assign yourself. Once that's done, you can remove the issue from the board (we do this to avoid duplicates). Thanks! |
This comment has been minimized.
This comment has been minimized.
Re: standup discussion: The code is just rezipping when a zip is a shapefile, but it is only including files with a known extension in doing so, so I agree that the one commit should do it. FWIW: I think at one point SEAD had a shapefile previewer that showed the contents on a map ala the geoJson previewer. I'm not sure if something like that would be useful for the group with these files or not (or whether opensource code to preview shapefiles would understand the qpj file, etc.) |
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Saixel I'd say you can safely switch this from draft to non-draft.
The tests are passing, which is great: https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-10305/3/testReport/
Do you plan to add or modify any tests?
Co-authored-by: Philip Durbin <philip_durbin@harvard.edu>
Yes! I was thinking of adding one for the specific case, do you think it's worth it? @pdurbin |
This comment has been minimized.
This comment has been minimized.
@Saixel yes, I think it would be great to add a test, especially if it doesn't involve adding any large test files. |
📦 Pushed preview images as
🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
The code looks good. API tests are passing. I also tested it. I used the When you unzip this file, its contents are:
Here's the before and after when uploading this file to Dataverse before: https://demo.dataverse.org server running Dataverse 6.1If you download
after: this pull requestIf you download
That is to say, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! See above for my writeup while testing.
What this PR does / why we need it:
This PR includes
.qpj
and.qmd
in the list of file extensions handled byShapefileHandler
, ensuring that QGIS project files and metadata are correctly processed and included in the zipped shapefile packages. The documentation has been updated to reflect these changes, and a new test has been added to confirm the proper handling of these file types.Which issue(s) this PR closes:
Special notes for your reviewer:
Please note that
.qpj
files are crucial for QGIS users as they contain projection information.Suggestions on how to test this:
.qpj
and.qmd
files.Does this PR introduce a user interface change? If mockups are available, please link/include them here:
N/A
Is there a release notes update needed for this change?:
Yes, it has been noted that Dataverse now retains
.qpj
and.qmd
files in zipped shapefile uploads.Additional documentation:
https://dataverse-guide--10305.org.readthedocs.build/en/10305/developers/geospatial.html