commons-vfs icon indicating copy to clipboard operation
commons-vfs copied to clipboard

Call `refresh` by default in `AbstractFileObject.onChange`

Open boris-petrov opened this issue 4 years ago • 13 comments

I think this is a good default. Most providers won't do anything different than this. I guess? @garydgregory thoughts?

boris-petrov avatar Dec 15 '20 12:12 boris-petrov

Hi @boris-petrov Run a local build before you create a PR, as this would have show you this change breaks the build. I am not sure of the semantics, needs studying... You PR would need a new test anyway to validate the change.

garydgregory avatar Dec 15 '20 12:12 garydgregory

@garydgregory - I spent a lot of time trying to figure what's going on. I give up. This test failure is definitely a bug in the RamFileProvider but it is difficult to fix. I changed a bunch of things and always something fails.

If you have the time, I would suggest you look into this and try fixing it.

Things that I saw which are not OK:

  1. AbstractFileObject::injectType is only overridden in RamFileObject which seems fishy. I would try to remove this method.

  2. AbstractFileObject::endOutput is a strange method. It is overridden only in RamFileObject which, again, is not nice.

Check also #151. I fixed some more "bad" things in the code around this.

boris-petrov avatar Dec 16 '20 15:12 boris-petrov

Hi @boris-petrov Please rebase on git master. TY!

garydgregory avatar Dec 16 '20 22:12 garydgregory

@garydgregory - done!

boris-petrov avatar Dec 17 '20 07:12 boris-petrov

@boris-petrov Please rebase on master to pickup better unit test assertion messages in ProviderWriteTests.

garydgregory avatar Dec 17 '20 14:12 garydgregory

@garydgregory - done!

boris-petrov avatar Dec 17 '20 14:12 boris-petrov

Hi @boris-petrov As you may have seen in the check list, this PR breaks the build:

Tests run: 93, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 5.007 sec <<< FAILURE! - in org.apache.commons.vfs2.provider.ram.test.RamProviderTestCase
testListener(org.apache.commons.vfs2.test.ProviderWriteTests)  Time elapsed: 0.007 sec  <<< FAILURE!
junit.framework.AssertionFailedError: ram:///write-tests/newfile

Please rebase on master to use the latest but I am thinking that it might be your actual changes that brake the test.

Also this PR does that have a test or tests that fail without your change to the main side of the source tree.

garydgregory avatar Dec 29 '20 14:12 garydgregory

@garydgregory - I rebased on master.

Please see this comment. As I said there, the failing test is a real issue in the RAM provider.

Also, please merge #151 before this. It simplifies some code and will help with debugging this issue here.

As for tests, this is not fixing any issues, it's just a convenience for some file systems that won't have to do anything "manually" onChange.

boris-petrov avatar Dec 29 '20 20:12 boris-petrov

Hi @boris-petrov May you rebase on master?

garydgregory avatar Apr 20 '22 15:04 garydgregory

@garydgregory done!

boris-petrov avatar Apr 20 '22 19:04 boris-petrov

@boris-petrov #151 is in, but the builds still fail here. Any ideas?

garydgregory avatar Apr 20 '22 21:04 garydgregory

Well, as I've said in a previous comment, there is some bug in the RamFileProvider. I've forgotten what the problem is. I guess someone would need to debug it.

boris-petrov avatar Apr 21 '22 07:04 boris-petrov

OK, then if someone wants to get this build green, they know where to dig.

garydgregory avatar Apr 21 '22 23:04 garydgregory