Refactor report_to_list #14
Conversation
* Refactor permissions check into can_change_or_view * Make group-by on multiple display fields work * Clean-up code around display field paths and totals * Generate values_list in a compatible (with history) way * Refactor sorting to use defaults rather than try/catch * Refactor choice_lists so that missing choices don't error * Refactor display_format with formatter function * Apply formatting to totals and arrange columns correctly * Simplify sort_helper based on new default-argument These changes should be entirely backwards compatible.
|
This pull request resolves Issue #6. This pull request also resolve issue 20 in django-report-builder. I'll have a separate pull requests with tests coming shortly. |
|
This is nice. Did you see the report_to_list branch I made? It might be better to focus efforts there instead. What do you think? |
|
I pulled to report_to_list since these are good tests. Clearly the report_to_list can't be merged as the approach is so different. The report_to_list branch is still far less code so I still think it's the best way forward. If the new report_to_list is going to take too long to reach feature parity - I could just merge this as is. |
|
I did see the report_to_list branch but I had a harder time following code. It looked like you moved a lot of the code to methods on Report. I can see that making sense but I'm wrapping up a project for a client this week so I ran with master. I don't have time to merge this yet with report_to_list but hopefully the refactoring is a big win. These changes should also be entirely backwards compatible. I can't verify that without tests but I strived not to change semantics at all. If you study the code, you'll see that lines 287 through 359 are actually verbatim. The diff fails to highlight this only because the indent level changed. I'm not using Custom Field nor am I familiar with it so I left it as-is. Everything else is pretty much just refactoring and code-shuffle. I do a couple things differently (like sorting with a default) but the semantics are un-changed. |
|
Let's merge as is then. Would you have any interest in helping maintain these two projects? Lots of pull requests all the sudden. |
|
Yes, I think so. I'm trying to close another contract that will use this shortly. Assuming that goes through, I will continue to have a vested interest in this project. |
|
Added - go ahead and merge these. When you're ready to make a pypi release I'll test against my own stuff and push to pypi. If you get that other contract - let me know what you think about the other branch. It seems like we can do the same stuff with far less code. A made report utils in an attempt to share more code between this and the not popular django-admin-export. But I ended up having report_to_list having a lot of report builder specific code which defeats the point. Never enough time for perfection. I'll have a new client using it too - hoping to get user stories from them soon. Should let me spend more time on this. |
Refactor report_to_list * Refactor permissions check into can_change_or_view * Make group-by on multiple display fields work * Clean-up code around display field paths and totals * Generate values_list in a compatible (with history) way * Refactor sorting to use defaults rather than try/catch * Refactor choice_lists so that missing choices don't error * Refactor display_format with formatter function * Apply formatting to totals and arrange columns correctly * Simplify sort_helper based on new default-argument These changes should be entirely backwards compatible.
|
Merged. Ready for PyPI release. |
These changes should be entirely backwards compatible.