-
Notifications
You must be signed in to change notification settings - Fork 66
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
Add apha file sort and hide hidden files via config.toml #526
base: master
Are you sure you want to change the base?
Add apha file sort and hide hidden files via config.toml #526
Conversation
fcollingwood
commented
Feb 6, 2022
- Added configuration option for alphanumeric sorting in the fille list rather than creation date/time
- Added configuration option for hiding hidden files (starting with".") from the file list
Added file_list_sort and show_hidden_files for use by mariner.server.api
Added config handlers for file_list -> sort_order file_list -> show_hidden
Changes the file list options to a table
Codecov Report
@@ Coverage Diff @@
## master #526 +/- ##
==========================================
- Coverage 81.77% 81.68% -0.10%
==========================================
Files 30 30
Lines 1240 1261 +21
Branches 44 44
==========================================
+ Hits 1014 1030 +16
- Misses 226 231 +5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is looking great! can you also add some test coverage? for the new config options you are setting up, you can take a look at how to write tests for them on the test_config.py
file. the tests for the file list are under test_server.py
# The sort order for the file list in the frontend. Can be: | ||
# * "alpha" - for Alphanumeric sort | ||
# * "creation" - for creation date/time sort | ||
#sort_order = "creation" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can you call this sort_by
instead? “order” implies in being able to set if it’s in ascending or descending order, while sort_by
makes it clearer that it’s about the field in which we are sorting
@@ -13,6 +13,16 @@ | |||
# directory that you are sharing to the printer through the USB Gadget | |||
files_directory = "/mnt/usb_share" | |||
|
|||
[file_list] | |||
# The sort order for the file list in the frontend. Can be: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update comment to reflect suggestion below
# * "creation" - for creation date/time sort | ||
#sort_order = "creation" | ||
|
||
# Show or hide files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Show or hide *hidden files
@@ -35,6 +35,24 @@ def get_files_directory() -> Path: | |||
return Path(str(config.get("files_directory", "/mnt/usb_share"))) | |||
|
|||
|
|||
def get_file_sort_order() -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of returning a str
here, can you return an enum? the main advantage is that it will be clear what are all the possible values that can be returned by this function - as opposed with a string where pretty much anything could be here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea how to, sorry.
path / dir_entry.name | ||
) | ||
|
||
if not (dir_entry.name.startswith(".") and not show_hidden_files): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you really need this logic twice on this function? wouldn’t it be possible to move this to the very beginning of the for loop as a guard clause and use continue
to skip hidden files? for example:
for dir_entry in sorted_dir_entries:
if dir_entry.is_file():
if dir_entry.name.startswith(“.”) and not show_hidden_files:
continue
# … code here stays as usual …
Had a look at the tests. Can't make head or tail of what they're supposed to be doing, so have no clue what you want there. |
…dden-files-via-config.toml