DRAFT: Order based on filenames or partial path #52
Conversation
Codecov Report
@@ Coverage Diff @@
## main #52 +/- ##
==========================================
- Coverage 97.79% 95.59% -2.21%
==========================================
Files 5 5
Lines 545 567 +22
Branches 121 129 +8
==========================================
+ Hits 533 542 +9
- Misses 8 17 +9
- Partials 4 8 +4
Continue to review full report at Codecov.
|
|
Thanks for that! I didn't get to this today, will probably have a closer look tomorrow (bit tired today, brain not working...), but at a first glance, this looks good to me. There is not much development philosophy to this project, other than keep it maintainable, don't add unneeded bloat, and have a test coverage close to 100% - after all, it's only a small project. If you want, you can add a couple of tests, or I can do that later. |
|
I haven't tried cleaning-up the patch or even running the tests so far, I mostly wanted to push that out for you to read first before doing anything else. Given your feedback, I guess I'll go a bit further on the next iteration, since it seems to be a 🆗 :) |
| for node_id in self.node_ids: | ||
| if node_id.count("::") == 2: | ||
| path_index = node_id.index("::") | ||
| if label.endswith('/') and label in node_id: # directory ordering |
There was a problem hiding this comment.
The in check here could be ambiguous - maybe node_id.startswith(label)? Or we need more checks here...
There was a problem hiding this comment.
I took the approach of checking endswith()in" in order to match any part of a directory's path, even if not fully specified (as it allows shorter and more concise specification in the decorator parameters).
There was a problem hiding this comment.
Yes, actually I did the same with endswith, which wouldn't work in your case, of course... given that it needs to end with a slash this may be ok (it would theoretically allow ambiguities, but the user would be responsible to avoid them).
| path_index = node_id.index("::") | ||
| if label.endswith('/') and label in node_id: # directory ordering | ||
| items.append(self.node_ids[node_id]) | ||
| elif node_id[:path_index].endswith(label): # end-of-filename ordering |
There was a problem hiding this comment.
This implies that there is only one directory ending with label, though it could be documented that label has to be unambiguous in that respect.
There was a problem hiding this comment.
I thought it would be easier to require ending the label with a / to refer to a directory, which would incidentally avoid confusing a directory and any other kind of path/entity.
|
@Joacchim - are you still working on this? Or are you waiting for more feedback? |
|
Ah, sorry, I've slacked-off on this so far. If you want to take it up, please do. |
|
Ok, thanks. This is currently not a priority for me, so it may take some time before I get to it, but I guess it isn't that important for you either. |
|
No, indeed. |
As discussed in #51, I've been fiddling with the module to try and see how I could fit my needs.
This is rough on the edges, and may not follow the project's development philosophy, but aims to be a first shot to be discussed and reflected upon.