#7949 Better listing of files changed in a certain commit
Followup to [#7925] sort of - at least things noticed from there.
Would be nice to show copies and renames a little differently. Not sure how though.
On a commit view for git, every parent directly is always listed as a change. This is due to -t in the git.diff_tree call. We should be able to replace this with -r (with some confirmation that added_paths() still works right)
Commit views should list all files in alphabetical order, rather than grouped by add/remove/etc. Make sure paginating through it works correctly still too.
Hey Dave,
Check this for the review
https://forge-allura.apache.org/u/pranav/allura/ci/pr/7949mod2/~/tree/
This works just right, I think. There's a few things to make sure the code is really good though.
The commits in
testui2.gitcould be more complicated so that the tests are stronger. With just one or two files in a commit, its hard to ensure that sorting really is working right. A good test case would be something like "add aaa.txt, remove bbb.txt, move ccc.txt to ddd.txt, add eee.txt" and check sorting then.The test that has
# Check for sorted output.isn't accurate, since sorting happens in the controller. Can we have a test to validate that? Would have to be a "functional" test and inspect the HTML (find some examples that user.html.find...which is a BeautifulSoup object and a good way to check for html elements).The pre-existing comment
# show tree entry itself as well as subtrees (Commit.added_paths relies on this)is something we should take note of carefully. The docstring inCommit.added_pathsalso mentions that it relies onpaged_diffsreturning all paths.added_pathsis used withinLastCommitwhich has some quite complicated logic (it determines what to show in the 'commit' column when browsing the repository directory structure). So, just to make sure we don't break anything there, I suggest you make an optional parameter onpaged_diffsto control whether to use-tor-rso that it still includes folder names in that case.Hey Dave, check the links for review of this one.
The branch is - https://forge-allura.apache.org/u/pranav/allura/ci/pr/7949mod2/~/tree/
The commit is - https://forge-allura.apache.org/u/pranav/allura/ci/833c1b892b001d81e39879f1f7bf7681bbe75f45/
Looking good, just a few things I want to suggest to make the code more readable & maintanable. The sorting line has a lot in it, and its not immediately obvious what it's doing, so a comment like "sort by filename" would help. Also it would be good to follow "pep8" formatting guidelines, e.g.
onlyChangedFiles=Trueinstead ofonlyChangedFiles = Trueand only one space beforeonlyChangedFiles(you have two spaces in a few places). I would recommend installing/configuring a pep8 tool for your editor. That will show you whenever your formatting doesn't follow the guidelines. (But don't worry about any existing violations, we just want new/changed code to follow the guidelines).ForgeHg will need to be updated as well since it has a
paged_diffsmethod. At the very least it needs to handle theonlyChangedFilesparameter being passed in and ignore it. Better would be to try an Hg repo and see if it includes directories and if so then make it skip them based on that parameter. https://sourceforge.net/p/forgehg/code/ci/master/tree/forgehg/model/hg.py#l523 If you can fork that repo and make a merge request for it, that'd be great.Nice work.
Oops, ForgeSVN needed the new param too. I've added it in [9585780494c181c2e5e46f1ec8622ad3bd600a7f]