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

Export ReadThrough in the csv export #3189

Merged
merged 11 commits into from
Jan 12, 2024

Conversation

ccamara
Copy link
Contributor

@ccamara ccamara commented Jan 3, 2024

This PR is an attempt (my first code contribution to bookwyrm and my first experience with Django -be warned!) to fix #2965 and include any book progress when exporting to CSV file.

@ccamara
Copy link
Contributor Author

ccamara commented Jan 3, 2024

Suggestions, comments and improvements are very welcome!

Copy link
Contributor

@hughrun hughrun left a comment

Choose a reason for hiding this comment

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

This works really nicely!

A couple of suggestions, hopefully they make sense but let me know if not.

bookwyrm/views/preferences/export.py Outdated Show resolved Hide resolved
bookwyrm/views/preferences/export.py Show resolved Hide resolved
@hughrun
Copy link
Contributor

hughrun commented Jan 4, 2024

You can fix the pylint error by adding to the disable comment at the top of the function:

# pylint: disable=no-self-use,too-many-locals
@method_decorator(login_required, name="dispatch")
class Export(View):

The test is failing because you're changing the output of the export file (which is the point!)

You should be able to adjust this test by adding the new headers, and three more commas to the next row (since there are no readthroughs in that test).

You can test whether this works locally by running:

./bw-dev pytest bookwyrm/tests/views/preferences/test_export.py

Let me know if you need help or advice with this. If you're feeling adventurous you could add a test to check the dates output properly, but it's not required.

@ccamara
Copy link
Contributor Author

ccamara commented Jan 4, 2024

Thanks again for the guidance @hughrun ! I've made multiple commits to address the issues you flagged!

These changes were introduced by mistake in my previous commit.
@ccamara ccamara requested a review from hughrun January 4, 2024 10:35
Copy link
Contributor

@hughrun hughrun left a comment

Choose a reason for hiding this comment

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

You're nearly there!

You can fix the linting errors by running ./bw-dev black to reformat, and adjusting the too-many-locals comment as per my note.

To check pylint won't complain you can run ./bw-dev pylint. It does take a little while so don't panic if it looks like your console is hanging.

bookwyrm/views/preferences/export.py Outdated Show resolved Hide resolved
@ccamara ccamara requested a review from hughrun January 6, 2024 08:59
@ccamara
Copy link
Contributor Author

ccamara commented Jan 6, 2024

Thanks again for this, @hughrun !

I believe my last commits should fix everything.

@hughrun hughrun merged commit b04ebe3 into bookwyrm-social:main Jan 12, 2024
10 checks passed
@ccamara ccamara deleted the 2965_export_readthrough branch January 12, 2024 07:58
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.

CSV export does not include read date
2 participants