Skip to content

Conversation

@aayushkdev
Copy link
Collaborator

fix #1319

Added the ability to export the current filtered QuerySet of a FilterView into the JSON format.

@aayushkdev aayushkdev force-pushed the 1319-add-json-format-export branch from 62b3ddb to 169c023 Compare February 12, 2025 06:31
@tdruez
Copy link
Contributor

tdruez commented Mar 11, 2025

@aayushkdev The implementation looks fine but we need a few unit tests before merging.

@aayushkdev
Copy link
Collaborator Author

Hey, thanks for the review. I have exams going on right now, so I'll add the unit tests as soon as they're finished.

@aayushkdev aayushkdev force-pushed the 1319-add-json-format-export branch 3 times, most recently from d8142a5 to e5e45ea Compare March 23, 2025 10:56
@aayushkdev
Copy link
Collaborator Author

Hey @tdruez, I have pushed the required unit tests. Could you please check if they are fit for merging

Copy link
Contributor

@tdruez tdruez left a comment

Choose a reason for hiding this comment

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

  1. The ExportJSONMixin is used for all the object models but only the CodebaseResourceListView returns a valid response.
    The code breaks if you try to export from any other list view, such as packages.
    The tests are incomplete as those only test for the resource type.

  2. The returned JSON is not valid when multiple records are included. It seems that the comma is missing.

@aayushkdev aayushkdev force-pushed the 1319-add-json-format-export branch 3 times, most recently from f439105 to d2b7497 Compare April 29, 2025 13:49
@aayushkdev
Copy link
Collaborator Author

Hey @tdruez I have pushed the required changes, please have a look.

@aayushkdev aayushkdev requested a review from tdruez April 29, 2025 15:27
Copy link
Contributor

@tdruez tdruez left a comment

Choose a reason for hiding this comment

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

@aayushkdev thanks for the changes, we are getting closer but the JSON output is still invalid. See the comment for suggestions.

Also on a more general note: avoid tempering with the git history by merging all previous commits and force-pushing in the branch, this makes looking at the code diffs since the last review much harder.
All the commits will be merged into a single commit once the PR is done, no need to rewrite history during the developement phase.

@aayushkdev
Copy link
Collaborator Author

Hey @tdruez I have made the required changes please have a look

@tdruez
Copy link
Contributor

tdruez commented Apr 30, 2025

@aayushkdev Everything looks good! Could you add a simple entry in the CHANGELOG.rst file so I can merge the PR?

@aayushkdev aayushkdev force-pushed the 1319-add-json-format-export branch from 1196f83 to 3b02bfe Compare April 30, 2025 11:38
@aayushkdev
Copy link
Collaborator Author

aayushkdev commented Apr 30, 2025

@aayushkdev Everything looks good! Could you add a simple entry in the CHANGELOG.rst file so I can merge the PR?

done!!

I have also rebased the commits into one

…rView' into the JSON format, Solves issue aboutcode-org#1319

Signed-off-by: Aayush Kumar <[email protected]>

made the implementation of ExportJSONMixin simpler by removing streaming of data and removed exclude_json_fields

Signed-off-by: Aayush Kumar <[email protected]>
@aayushkdev aayushkdev force-pushed the 1319-add-json-format-export branch from 3b02bfe to 4be8585 Compare April 30, 2025 11:50
@tdruez
Copy link
Contributor

tdruez commented Apr 30, 2025

I have also rebased the commits into one

@aayushkdev As mentioned earlier, you never need to do this, and should never force-push. It's valuable to keep the commit history.
The merge into main is done as a "Squash and merge", which appears as a single commit in the main branch, but we want to keep each single development commit untouched.

@tdruez tdruez merged commit 81e05b9 into aboutcode-org:main Apr 30, 2025
5 checks passed
@tdruez
Copy link
Contributor

tdruez commented Apr 30, 2025

@aayushkdev Thanks for your contribution! But keep in mind, if you force-push, you are doing it wrong ;)

@aayushkdev
Copy link
Collaborator Author

I have also rebased the commits into one

@aayushkdev As mentioned earlier, you never need to do this, and should never force-push. It's valuable to keep the commit history. The merge into main is done as a "Squash and merge", which appears as a single commit in the main branch, but we want to keep each single development commit untouched.

Ah, I see - thanks for clarifying. I misunderstood and thought the final rebase was expected. I’ll make sure not to force-push or squash commits like this again. Appreciate the guidance!

@aayushkdev aayushkdev deleted the 1319-add-json-format-export branch April 30, 2025 12:23
@tdruez
Copy link
Contributor

tdruez commented Apr 30, 2025

@aayushkdev No worries. Thanks for this super useful feature!

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.

Add JSON format in list view export

2 participants