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

🛬 [INBOUND] - drf-excel #45

Open
3 tasks done
FlipperPA opened this issue Sep 26, 2024 · 10 comments
Open
3 tasks done

🛬 [INBOUND] - drf-excel #45

FlipperPA opened this issue Sep 26, 2024 · 10 comments

Comments

@FlipperPA
Copy link
Member

FlipperPA commented Sep 26, 2024

What's the URL to the project?

https://github.com/wharton/drf-excel

Who are the current maintainers?

Timothy Allen, Mathieu Rampant

Does the project have the required attributes?

  • Does the project have tests?
  • Does the project have documentation?
  • Has the project adopted Django Commons's Code of Conduct?

Who will be the new maintainers of the project?

We've got to wrap up a rudimentary test suite.

@tim-schilling
Copy link
Member

Linking the drf-excel's issue for posterity. wharton/drf-excel#86 Thank you Tim for opening this!

@FlipperPA
Copy link
Member Author

Thanks to @browniebroke, we now have some rudimentary tests in place, @tim-schilling. Happy to transfer in when you are ready.

@tim-schilling
Copy link
Member

@FlipperPA can you confirm that this is the only test in the project? I've been trying to find others, but it seems like the work was to get a test harness up and running with CI. If I'm wrong, potentially ignore the rest of my comment.

If that's the case, the concern there is a new person can't come in and change things confidently. Maybe a decent middleground here is to write up two issues, 1 to add code coverage and 2 identify the main components that are critical to having tests. I wonder if we can pin an issue in a repo to help encourage efforts toward them.

I think if two of the other @django-commons/admins are good with that (and there aren't any strong arguments against), we can bring it in.

I don't want to slow things down unnecessarily, but I do feel like tests need to be prioritized for drf-excel to help others contribute.

@FlipperPA
Copy link
Member Author

FlipperPA commented Sep 30, 2024

@tim-schilling Thanks to the efforts of @browniebroke, there's now a more robust test for the full XLSX workbook generation routine. https://github.com/wharton/drf-excel/blob/main/tests/test_viewset_mixin.py

I'm ready to move it in once you are comfortable. I'm still kicking myself a bit for having much better test coverage on my projects that get barely used!

@cunla
Copy link
Member

cunla commented Oct 4, 2024

@FlipperPA, I agree with @tim-schilling.

You can add coverage visibility to in the test github workflow @browniebroke implemented (See example here for publishing a coverage report as a comment in the PR).

Once there is coverage visibility, it should be easy to identify what components require more robust testing.

@browniebroke
Copy link
Member

See example here for publishing a coverage report as a comment in the PR).

I don't really understand where the coverage is displayed with the solution, I looked at recent PRs, but they don't have a comment displaying coverage.

Regarding drf-excel, the coverage is currently displayed in the logs of GitHub actions (just in case you missed it).

On the other hand, I see that django-typer uses codecov, which is much easier to setup. I gave this a go: wharton/drf-excel#90 I also have a follow up piece where I'm bumping coverage a bit: browniebroke/drf-excel#3

@tim-schilling
Copy link
Member

I did miss that. I'm good with this being merged into the org.

@FlipperPA
Copy link
Member Author

@tim-schilling How does the actual merge happen? Do I just move the repo myself? Thank you!

@browniebroke
Copy link
Member

I think that's documented here? https://github.com/django-commons/controls#new-project-playbook

PS: I've joined the drf-excel maintainance team since this issue was opened

@tim-schilling
Copy link
Member

@FlipperPA @browniebroke one of the admins will reach out to schedule a time to do the transfer. Right now our transfers are clocking in at about 1.25 hours.

One thing you can do ahead of time that will help is answering the following questions:

  • Does the project have a test.pypi.org project?
  • Would you like to test your deployments with uploads to test.pypi.org (you'll need to create the project first)?
  • Would you like to use GitHub's merge commits?
  • Would you like to use GitHub's rebase and merge?
  • Would you like to use GitHub's squash and merge?
  • Would you like to use GitHub's Discussions?
  • Would you like to use GitHub's Wiki?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants