--currentdir functionality, resubmitted from current master branch

[ghost]

Resubmitted https://github.com/andreafrancia/trash-cli/pull/306 from new master branch as there were issues with old pull request.

Based on issue https://github.com/andreafrancia/trash-cli/issues/304

(Also a minor change from last time, it now won't print the current directory if there was a previous directory with the same name that was trashed)

[andreafrancia]

Hi, thank you for your submission.

The change from the last time are all ok.

Some test are failing. If you mind try to launch them and check what is wrong. You can refer to the Development section of the README to learn how to launch them.

We also need a test for the new functionality. It can be added, for example in test_trash_list.py. You will need to inject the value of the current working directory without letting the code get it directly from os.getcwd() to make the test pass.

[ghost]

Hi, thank you for your submission.

The change from the last time are all ok.

Some test are failing. If you mind try to launch them and check what is wrong. You can refer to the Development section of the README to learn how to launch them.

We also need a test for the new functionality. It can be added, for example in test_trash_list.py. You will need to inject the value of the current working directory without letting the code get it directly from os.getcwd() to make the test pass.

I fixed the existing test breaking, it was because of the new command line option changing the help text.

For the new tests, is it okay to use os.getcwd() in the tests like this:

    def test_should_output_only_currentdir_with_currentdir_option(self):
        self.user.home_trashdir.add_trashinfo2(os.getcwd() + '/absolute/path/file1',
                                               datetime(2001, 2, 3, 23, 55, 59))
        self.user.home_trashdir.add_trashinfo2('/absolute/path/file2',
                                               datetime(2001, 2, 3, 23, 55, 59))

        self.user.run_trash_list('--currentdir')

        assert_equals_with_unidiff("2001-02-03 23:55:59 " + os.getcwd() + "/absolute/path/file1\n",
                                   self.user.output())

Doing that does work on my system, and the tests passed when I pushed it to my forked repo.

[andreafrancia]

If I remember well the test is using a fake filesystem, so I don't know if it will work. You can try.

[ghost]

It seems to have worked. I added two tests, one testing the basic functionality (i.e. only showing trashed files from the current directory), and another ensuring that the directory itself is not included.

[andreafrancia]

Thank you, I'll review the code as soon as possible. Probably I'll try also to remove os.getcwd() reference from the tests and substitute them with a basic string, to improve the expressiveness of the test (DRY vs DAMP). And you wish to inject the value of os.getcwd() in the trash-list production code. I don't remember if already did something like that in trash-restore.

Your code is ok, but I want to add these two things before merging it.

If you want you can add these two things by yourself, otherwise I will add them as soon as I have time to do it.

Thank you

[ghost]

Thank you, I'll review the code as soon as possible. Probably I'll try also to remove os.getcwd() reference from the tests and substitute them with a basic string, to improve the expressiveness of the test (DRY vs DAMP). And you wish to inject the value of os.getcwd() in the trash-list production code. I don't remember if already did something like that in trash-restore.

Your code is ok, but I want to add these two things before merging it.

If you want you can add these two things by yourself, otherwise I will add them as soon as I have time to do it.

Thank you

I see what you mean with the tests. I changed the implementation a bit so the cwd is passed in, similar to restore. Also put the tests in a separate file and the cwd is set in the setUp part of the test.

[andreafrancia]

I reviewed your code and I think that should tested also with volume trashdirs, and revisiting the support code for tests easier do to such tests. It should also be tested with python 2.7.

[andreafrancia]

Unfortunately some changes I made for enhancing the test support in trash-list introduced some new conflicts.

[ghost]

Hi, sorry I've been inactive the last couple of weeks, have some exams at Uni at the moment. I'll be done soon, can take another look at the code then.

[andreafrancia]

Take your time, no hurry. Pull Requests are always welcome.

[andreafrancia]

I'm closing this PR in order to clean things.

Send me a message when you want to reopen it.

[ghost]

Hi, sorry I forgot to come back to this. I think I have resolved the conflicts and got the tests working again.

For the volume trashdirs, I duplicated the existing tests I had but with a fake disk (u.add_disk()).

I installed Python 2.7 from the AUR, I had to use an older version of virtualenv to get the virtual environment to work:

pipx install virtualenv==20.21.1
virtualenv -p =/usr/bin/python2.7 venvLoc
venvLoc/bin/pip install -r requirements.txt
venvLoc/bin/pip install -r requirements-dev.txt
venvLoc/bin/python2 -m pytest

It appears to be working, all the tests pass.

Let me know if there is anything else to be done.

(The changes aren't showing up as the PR is closed, they should be on my fork)

[andreafrancia]

Hi, thank you.

Unfortunately it seems that you deleted your account from GitHub before I had a chance to reply this last message.