Clinical-Genomics / housekeeper

File data orchestrator
MIT License
2 stars 0 forks source link

[On Hold] Do Not Allow Directories To Be Added #78

Closed mikaell closed 1 year ago

mikaell commented 4 years ago

Summary

Testing

alt1 $ pytest test alt2 try to add a directory

$ housekeeper add file myBumdle my_directory
my_directory is a directory
Aborted!

Review

This version is a:

coveralls commented 4 years ago

Coverage Status

Coverage increased (+0.09%) to 88.508% when pulling f388b77e444e9c09ac8badb3067ae6529eb82b52 on fix-no_dirs-200908 into 0b8dda38cee1229f99412abe23b6a6f7b9e79d13 on master.

Mropat commented 4 years ago

Very nice solution! However, I think we could benefit from catching the problem at its core, in AddHandler.new_file(). This method is called when any file is being added by any means to Housekeeper. The checks on path existing and being a directory can be performed there, and an appropriate error can be raised - then caught in the CLI.

mikaell commented 4 years ago

However, I think we could benefit from catching the problem at its core, in AddHandler.new_file().

The implementation was done analogous with other sanity checks. For example: A. Check if the file/path exists. This is done in 'add.py'.

add.py:
    if not path.exists():
        click.echo(click.style(f"File {path} does not exist", fg="red"))
        context.abort()

B. Check if bundle exists

    bundle_obj = store.bundle(bundle_name)
    if bundle_obj is None:
        click.echo(click.style(f"unknown bundle: {bundle_name}", fg="red"))

AddHandler performs no sanity checks at the moment. I haven no problem moving them. But then every sanity check should be centralised. Personally I think user I/O errors should be caught early, in the code handling I/O, leaving internal representation clean.

Mropat commented 4 years ago

@mikaell AddhHandler.add_bundle is not raising any errors for bundle and is expected to behave this way right now by cg (where it is imported directly) unfortunately. This behavior for now is also what would be desirable to keep when adding bundle/version through cg when version contains files that are folders. Containing the check to CLI wont fix the biggest problem however, when the main mode of adding files that are folders to HK is by adding it as a part of a version in CG.

This means then, that raising error is not desirable, but only introduce a check for folder, print a log warning, and return nothing if folder is being added! I still think it would be best to handle once and for all in AddHandler, but perhaps @barrystokman and/or @moonso could have a different take on this

mikaell commented 4 years ago

Pausing the pull-request until the CLI rewrite is published.

ChrOertlin commented 1 year ago

closing - out of date - new issue created https://github.com/Clinical-Genomics/housekeeper/issues/153