File creation fix #427
File creation fix #427
Conversation
library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for such a good change. Looks good to me.
Only a few minor comments with suggestions for micro-optimizations.
Also, I see a typo in PR description with wrong PR mentioned in the first part in that was the cause of #420. Please update it as well to have proper history descriptions.
library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/support/TeeSource.kt
Outdated
Show resolved
Hide resolved
|
Ok, I applied your suggestions. I would appreciate it if you can give feedback on #427 (comment) from @koral-- and how you'd like to handle it. |
|
Coverted it to a draft because I discovered some issues. |
|
In cbedcbc I decoupled tee operation from reporting functionality. Reporting is now a part of the I can remove it from this PR and make a different one with just this commit if you want me to. Or remove it completely but I'm happy now with how reporting bytes and copying bytes is no longer coupled. |
library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/support/ReportingSink.kt
Outdated
Show resolved
Hide resolved
library/src/test/java/com/chuckerteam/chucker/internal/support/ReportingSinkTest.kt
Outdated
Show resolved
Hide resolved
library/src/test/java/com/chuckerteam/chucker/internal/support/ReportingSinkTest.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/support/ReportingSink.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt
Outdated
Show resolved
Hide resolved
|
I added the suggested changes. CI fails for now because I added a functional interface and #438 needs to be merged first. I'll rebase after. Unless you think that |
- Remove FileFactory interface in favor of concrete class implementation that accepts files directory. - In case of IO failures or missing directory user gets feedback in Logcat or via Toast.
…ceptor.kt Co-authored-by: Nicola Corti <[email protected]>
Since we're on 1.4, let's go with a functional interface 👌 |
library/src/main/java/com/chuckerteam/chucker/internal/support/CacheDirectoryFactory.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/support/CacheDirectoryFactory.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt
Outdated
Show resolved
Hide resolved
|
Great, thanks! I added one test for a missing cache in the first place. |
📄 Context
There are open issues - #422, #414.
📝 Changes
FileFactoryis now IO safe and typesafe. I removed the interface in favor of a concrete implementation. It was internal anyway and it was misleading in tests ofChuckerInterceptorbecause fake implementation did not take into account stuff likecreateNewFile(), that was the cause of #422.One more thing that I changed in the implementation of the factory is that access to the parent directory. It is now lazy, so it should help with #413 since the cache directory will no longer be accessed during initialization. It won't fix it as there are other files being used on the main thread but it is one step forward.
I'm not particularly happy about the fact that- This isn't the case anymore. See #427 (comment).TeeSourceaccepts nowFile?as an input parameter but I didn't want to overcomplicate things by introducing another source that just counts bytes downloaded.📎 Related PR
I copied the test for the file factory from #423.
🚫 Breaking
No.
🛠️ How to test
In practice not sure. Maybe @CodeBreak524 can test with a dependency from my branch on JitPack (
com.github.MiSikora.chucker:library:file-creation-fix-SNAPSHOT). I didn't write new tests forChuckerInterceptoras old ones cover this case due to using real file factory in tests.⏱️ Next steps
Closes #422
Closes #414
Closes #423