This diff adds an option --cwd-list to hg sparse. This will return the
contents of the current directory. The files that are in the sparse profile are
annotated with a '-' indicator.
Details
- Reviewers
durham akushner mitrandir - Group Reviewers
Restricted Project - Commits
- rFBHGX70f4fed0e5dd: sparse: add a --cwd-list option
Tested by running the command 'hg sparse --cwd-list' in various folders.
Diff Detail
- Repository
- rFBHGX Facebook Mercurial Extensions
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
hgext3rd/sparse.py | ||
---|---|---|
710–711 | How are the files in the profile indicated compared to the ones that aren't in the profile? This isn't clear to someone reading how the output is annotated. | |
1062 | Comments should tell the "why" not "what". The code tells that you are getting the "root". The "why" is used to explain something that might not be obvious to the reader or why a specific decision was made. | |
1075–1076 | These comments aren't needed. | |
1077 | if len(cwd)+1 is a constant, generally best left outside a loop (although in a short list of files, probably not noticeable. | |
1078 | Will this work on WIndows? | |
1086 | perhaps for folder in sorted(all_folders): When you type 'ls', notice how the default is alphabetic sorting? If someone is going to compare the output of this command with what's in the directory, it should be in the same sorted order. I can even see a case where all the '-' annotated ones are at the top or bottom, but interspersed with no ordering makes this tool hard to use. | |
1087 | This is shorter and keeps alignment marker = ' ' if folder in checkedout_folders else '-' repo.ui.status("%s %s\n" % (marker, folder) |
hgext3rd/sparse.py | ||
---|---|---|
667 | I think --cwd-list might be a better name for the flag. find implies searching for something, which isn't quite what this is doing. | |
1054 | Mercurial variable naming avoids mid-variable underscores (they differ from python on this), so I'd rename this _cwdfind (or _cwdlist). | |
1060 | Since files is only ever iterated over, and since mf will never have duplicates, you don't need to create the set() here. Just iterate of 'mf' instead (which is what set() is doing here to create the set). | |
1061 | I think we need to call cwd = util.normpath(cwd) on this to normalize it. That will turn windows paths into unix looking paths, to address Aaron's question about windows below. | |
1068 | I think there needs to be a space after 'begin'. I'd also lowercase the initial 'the', since that's mercurial style. | |
1072 | I'd drop the underscores, for style consistency. | |
1077 | I think this +1 is the reason the first character is missing when listing the root directory. So len(cwd) is constant, but the +1 might not be. | |
1088 | I'd definitely have two spaces before the normal directories. So they align nicely with the '- ' directories in the output. Aaron's proposal does that as well |
It looks pretty good. The last thing would be to add a test. There are already some sparse tests in the tests directory, so it should be easy to add some new lines there. Let me know if you'll be busy on other tasks, and I can chip in tests to make sure this gets landed.
hgext3rd/sparse.py | ||
---|---|---|
1059 | The appropriate name for this is still mf, since it is a manifest instance. |
Successfully pushed the code to the repo - https://phabricator.intern.facebook.com/P58345906. Closing the task.
I think --cwd-list might be a better name for the flag. find implies searching for something, which isn't quite what this is doing.