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

feature/mx-1632 structured file support #134

Merged
merged 45 commits into from
Sep 26, 2024

Conversation

ZehraVictoria
Copy link
Contributor

@ZehraVictoria ZehraVictoria commented Aug 1, 2024

PR Context

So far mex drop only allows JSON file upload.
This establishes compatibility with most common structured file formats.


Added

  • support for more structured file formats

@ZehraVictoria ZehraVictoria marked this pull request as ready for review August 1, 2024 11:35
Signed-off-by: Zehra Ciftci <[email protected]>
@cutoffthetop cutoffthetop changed the title mx-1632 structured file support feature/mx-1632 structured file support Aug 12, 2024
Signed-off-by: Zehra Ciftci <[email protected]>
@ZehraVictoria ZehraVictoria force-pushed the feature/mx-1632-structured-file-support branch from d03fe2a to 31b3069 Compare August 13, 2024 12:43
Signed-off-by: Zehra Ciftci <[email protected]>
Signed-off-by: Zehra Ciftci <[email protected]>
Signed-off-by: Zehra Ciftci <[email protected]>
Signed-off-by: Zehra Ciftci <[email protected]>
Signed-off-by: Zehra Ciftci <[email protected]>
Signed-off-by: Zehra Ciftci <[email protected]>
Signed-off-by: Zehra Ciftci <[email protected]>
Copy link

@mr-kamran-ali mr-kamran-ali left a comment

Choose a reason for hiding this comment

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

Nice work. Few little things to fix.

requirements.txt Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
mex/drop/api.py Outdated Show resolved Hide resolved
Signed-off-by: Zehra Ciftci <[email protected]>
Signed-off-by: Zehra Ciftci <[email protected]>
Signed-off-by: Zehra Ciftci <[email protected]>
tests/test_main.py Outdated Show resolved Hide resolved
Signed-off-by: Zehra Ciftci <[email protected]>
Copy link
Contributor

@rababerladuseladim rababerladuseladim left a comment

Choose a reason for hiding this comment

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

Nice work! I do have a few remarks.

First and foremost: One test fails locally: tests/test_main.py::test_index[firefox]

CHANGELOG.md Outdated Show resolved Hide resolved
tests/test_api.py Show resolved Hide resolved
mex/drop/api.py Outdated Show resolved Hide resolved
mex/drop/api.py Outdated Show resolved Hide resolved
mex/drop/api.py Outdated Show resolved Hide resolved
tests/test_api.py Show resolved Hide resolved
tests/test_api.py Outdated Show resolved Hide resolved
tests/test_api.py Outdated Show resolved Hide resolved
mex/drop/api.py Outdated Show resolved Hide resolved
mex/drop/api.py Show resolved Hide resolved
Copy link
Contributor

@rababerladuseladim rababerladuseladim left a comment

Choose a reason for hiding this comment

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

Nice, almost done.

A few remarks:

  • could you move the helper functions and variables to a dedicated module, e.g. files_io.py?
  • I struggled to understand which conversations where fully addressed. To make this easier, we usually work with conversations as follows:
    • the reviewer closes conversations when she/he deems the issues fully addressed
    • the developer adds a comment with the hash of the commit that includes the fix (this facilitates finding of the fix and spotting which conversations you already replied to)

One major remark:
I tried to upload a csv file and get the following error:

{
  "detail": "Content doesn't match extension: application/vnd.ms-excel != Joplin-Setup-3.0.15.csv"
}

I'm not sure who sets that content type, I assume it's the browser. Certainly not me.

mex/drop/api.py Outdated Show resolved Hide resolved
mex/drop/api.py Outdated Show resolved Hide resolved
mex/drop/api.py Outdated Show resolved Hide resolved
mex/drop/api.py Outdated Show resolved Hide resolved
tests/test_api.py Outdated Show resolved Hide resolved
tests/test_api.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
mex/drop/api.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
mex/drop/api.py Outdated Show resolved Hide resolved
@rababerladuseladim
Copy link
Contributor

And one more: could you document the allowed file types in swagger as well? That makes using the API easier.

@ZehraVictoria
Copy link
Contributor Author

ZehraVictoria commented Sep 10, 2024

One major remark: I tried to upload a csv file and get the following error:

{
  "detail": "Content doesn't match extension: application/vnd.ms-excel != Joplin-Setup-3.0.15.csv"
}

I'm not sure who sets that content type, I assume it's the browser. Certainly not me.

It seems csv files edited in MS Excel are recognized as application/vnd.ms-excel MIME-type instead of text/csv.
Fixed in 5c69fc5

And one more: could you document the allowed file types in swagger as well? That makes using the API easier.

Also added 5c69fc5. I didn't find anything useful on how to limit the file types in the file selecter popup though

* could you move the helper functions and variables to a dedicated module, e.g. `files_io.py`?

fixed in 35bffdf

Copy link
Contributor

@rababerladuseladim rababerladuseladim left a comment

Choose a reason for hiding this comment

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

Biu-ti-ful! And almost there, just some tiny nitpicking left.

mex/drop/api.py Outdated Show resolved Hide resolved
mex/drop/api.py Show resolved Hide resolved
mex/drop/files_io.py Outdated Show resolved Hide resolved
Copy link
Contributor

@rababerladuseladim rababerladuseladim left a comment

Choose a reason for hiding this comment

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

oh boi, just one tiny thing left.

mex/drop/api.py Show resolved Hide resolved
@cutoffthetop cutoffthetop dismissed rababerladuseladim’s stale review September 26, 2024 13:33

requested changes applied

@ZehraVictoria ZehraVictoria merged commit 8026f71 into main Sep 26, 2024
4 checks passed
@ZehraVictoria ZehraVictoria deleted the feature/mx-1632-structured-file-support branch September 26, 2024 14:07
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.

4 participants