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

Convert phase: Adds saving converted input as a csv #149

Merged
merged 7 commits into from
Nov 13, 2023

Conversation

CarlosCoelhoSL
Copy link
Contributor

This PR adds functionality to the convert phase so that when the input file (geojson, xlsm etc) is converted to a .csv file, this is saved to disk (rather than a temporary file). The location of where the file is saved is determined by arguments for the ConvertPhase class. If these arguments aren't present then the file isn't saved to disk.

This is required so the pipeline-runner can use the converted input .csv as part of its response.

Copy link
Contributor

@eveleighoj eveleighoj left a comment

Choose a reason for hiding this comment

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

Needs to be more throughly testing with the core file types we ask for from LPAs. Approach should be change to give a different file name in the convert_features_to_csv function.

tests/unit/test_convert.py Outdated Show resolved Hide resolved
digital_land/phase/convert.py Outdated Show resolved Hide resolved
digital_land/phase/convert.py Outdated Show resolved Hide resolved
Removes old method of saving converted .csv to disk
@@ -13,3 +13,55 @@ def test_load_xlsm():
assert block["line-number"] == 1
assert "OrganisationURI" in block["line"]
assert log.mime_type == "application/vnd.ms-excel"

Copy link
Contributor

Choose a reason for hiding this comment

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

So it's not your fault that this test doesn't exist but we need a test that checks what happens when a custom_temp_dir is not used. I.e. if the user of the function does not want to to store the data in a directory they can find later then what happens.

Your current code would still create the files in the converted folder.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another test to write would be one that could test for data not being in the converted folder and instead being in a user data

@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (ac51e9c) 79.43% compared to head (252f0d9) 79.44%.
Report is 6 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #149   +/-   ##
=======================================
  Coverage   79.43%   79.44%           
=======================================
  Files          67       67           
  Lines        3594     3600    +6     
=======================================
+ Hits         2855     2860    +5     
- Misses        739      740    +1     
Files Coverage Δ
digital_land/phase/convert.py 74.58% <92.30%> (+0.29%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CarlosCoelhoSL and others added 2 commits November 10, 2023 12:15
@eveleighoj eveleighoj merged commit 7dcf8c6 into main Nov 13, 2023
@eveleighoj eveleighoj deleted the save-converted-input-file branch November 13, 2023 12:24
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.

3 participants