File & directory deletion feature

[cyqsimon]

Closes #397.

Todo

• CLI options
• Backend API
• Frontend controls
• Tests

[cyqsimon]

miniserve/src/main.rs

Lines 329 to 330 in 82dd826

	// Allow file upload
	app.service(web::resource("/upload").route(web::post().to(file_upload::upload_file)));

I am curious why the upload feature is implemented like so. I guess it's good for unforeseen future extensibility, but there is semantic duplication between the /upload endpoint and the POST method.

The real question I want to ask, is should I implement the deletion feature in a similar manner? I.e. something like this:

app.service(web::resource("/delete").route(web::delete().to(file_rm::rm_file)));

[svenstaro]

Are you suggesting it might have been better to just have POST always try to upload stuff? It would be simpler for sure but as you are saying, it's not great if we ever want a second POST for something. I think this is currently an ok compromise.

I see how this is a bit of a conundrum with delete. I also want to add WebDAV at some point and that will introduce even more semantics. Perhaps it's the best to go with what you suggested just now and eat the semantic duplication for the time being. It's not like it can't be changed later.

[bgusach]

Hi,

unfortunately I can't directly help... just wanted to say that I appreciate the effort and if it gets done at some point, my life would be somehow better :)

[cyqsimon]

Hi,

unfortunately I can't directly help... just wanted to say that I appreciate the effort and if it gets done at some point, my life would be somehow better :)

Thanks for reminding me. I have too many things on hand and honestly just forgot about this whole matter. I will find some time to work on it this week, hopefully.

[cyqsimon]

I suck at visual design. So any styling improvement suggestion is welcomed.

[cyqsimon]

Rebased on #1331.

[cyqsimon]

Rebased on master.

[cyqsimon]

Rebased on master.

[svenstaro]

Could you add some tests? Since this is going to be deleting things, it needs some proper tests.

[cyqsimon]

Could you add some tests? Since this is going to be deleting things, it needs some proper tests.

Yeah I'm working on that on and off. The tests are actually written but several are failing. My original intention was to combine the tests and the fixes into one commit, but I guess there's no harm in splitting them up. I'll push the tests first and then look to fix them.

[cyqsimon]

If you can spare some time, please take a look at the test suite; make sure they are testing the correct behaviour. Thanks.

[svenstaro]

Yeah I'm working on that on and off. The tests are actually written but several are failing. My original intention was to combine the tests and the fixes into one commit, but I guess there's no harm in splitting them up. I'll push the tests first and then look to fix them.

No worries. There's no rush to get this in. Just take your time and make sure things look good.

If you can spare some time, please take a look at the test suite; make sure they are testing the correct behaviour. Thanks.

I will try to squeeze a review in but no promises, I'm fairly low on time currently.

[cyqsimon]

Rebased on #1454.

[cyqsimon]

This should be ready now, finally. Everything works as expected, as far as I can tell. But you may wish to merge the other PR first.

[svenstaro]

Needs a rebase and then I will do a proper review. :)

[cyqsimon]

Actually, let me do an interactive rebase to reorganise things. Then it'd be a bit tidier.

[qtekk]

hey, how's the feature going?

[cyqsimon]

hey, how's the feature going?

Implementation is complete. Just waiting on review.

@svenstaro You got time to take a look?

[svenstaro]

I will take some time. Might take a look on the weekend.