Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate cache hash length and folder depth #212

Conversation

ronaldbarendse
Copy link
Contributor

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

As discussed in #210, this separates the cache hash length and (PhysicalFileSystemCache specific) cache folder depth. It also generates shorter cache file names by default by not repeating the characters that are already used in the nested folders.

@codecov
Copy link

codecov bot commented Feb 1, 2022

Codecov Report

Merging #212 (ff55d06) into master (fe0b4e9) will increase coverage by 2%.
The diff coverage is 96%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #212    +/-   ##
======================================
+ Coverage      86%    89%    +2%     
======================================
  Files          68     65     -3     
  Lines        1791   1653   -138     
  Branches      262    232    -30     
======================================
- Hits         1556   1482    -74     
+ Misses        167    112    -55     
+ Partials       68     59     -9     
Flag Coverage Δ
unittests 89% <96%> (+2%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../ImageSharp.Web/Caching/PhysicalFileSystemCache.cs 97% <95%> (+<1%) ⬆️
...harp.Web/Caching/PhysicalFileSystemCacheOptions.cs 100% <100%> (ø)
src/ImageSharp.Web/Caching/SHA256CacheHash.cs 100% <100%> (ø)
...DependencyInjection/ServiceCollectionExtensions.cs 100% <100%> (ø)
.../ImageSharp.Web/Middleware/ImageSharpMiddleware.cs 85% <100%> (ø)
...harp.Web/Middleware/ImageSharpMiddlewareOptions.cs 90% <100%> (ø)
...ching/LruCache/ConcurrentTLruCache{TKey,TValue}.cs
...hing/LruCache/LongTickCountLruItem{TKey,TValue}.cs
...ching/LruCache/TLruLongTicksPolicy{TKey,TValue}.cs

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe0b4e9...ff55d06. Read the comment docs.

@ronaldbarendse
Copy link
Contributor Author

I've excluded the LRU cache from code coverage for now: I'm not sure why the coverage decreased, but it's completely missing dedicated tests to begin with.

@JimBobSquarePants
Copy link
Member

@ronaldbarendse Don't worry about the coverage and don't hide it. It's flaky as hell. I had a change recently observed when converting to png's to LFS pointers

@ronaldbarendse
Copy link
Contributor Author

Because I think adding a setting just for backwards compatibility isn't the best way forward, I thought about creating a sub-class called LegacyV1PhysicalFileSystemCache (similar naming as the LegacyV1CacheKey). This could then simply set the base class useLegacyName boolean using a protected constructor. But even this introduces code that needs to be cleaned up in the future, so I decided on using the fact that the cache depth and hash length are currently the same to revert back to creating the legacy/longer file paths: #213.

@ronaldbarendse ronaldbarendse deleted the feature/cachehash-cleanup branch February 2, 2022 21:41
@JimBobSquarePants
Copy link
Member

I think closing this PR and opening another was a bit premature. Best leaving those kinds of decisions to the library owners.

@ronaldbarendse
Copy link
Contributor Author

Fair enough, although I was pretty sure the other PR that doesn't require additional settings would be preferred 😇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants