Add variable expansion in defaultDestination#759
Conversation
jimmylewis
force-pushed
the
defaultDestTemplate
branch
from
c929c6b to
ff5e6f5
Compare
| int cutIndex = name.LastIndexOfAny(['/', '\\']); | ||
|
|
||
| StringBuilder stringBuilder = new StringBuilder(destination); | ||
| stringBuilder.Replace("[Name]", cutIndex == -1 ? name : name.Substring(cutIndex + 1)); |
There was a problem hiding this comment.
Possibly a non-real case, but what if name contains [Version]? That would cause the next line to do an extra/unexpected replacement.
There was a problem hiding this comment.
At least as far as NPM is concerned, I don't believe that is a valid name (only allows alphanumeric, underscores, and hyphens). Since that's the source for the non-filesystem providers (jsdelivr and unpkg directly use NPM, cdnjs is indirectly curated from NPM), it would be an edge case for filesystem only. I thought about making this logic provider-specific, but that was a lot more complicated than doing it here centrally.
I did briefly consider using a different token that wouldn't be allowed in file paths (implicitly compatible with all providers since they all come down to file names), but on Linux the only character is /. I'm definitely open another delineator if you have a preference, but [Version] felt most natural (next on my list was something like {{Version}}); it should be a pretty trivial change.
This implements aspnet#68, except I used [Name] instead of [LibraryName]. It felt like LibraryName should be matched with LibraryVersion and that felt verbose, so I took the shorter versions. This expansion is applied when we expand the ManifestOnDisk (which is either read from disk or from a raw JSON) into LibraryInstallationState. This is where we determine to use the defaultDestination or a library-specific destination, so it should be the only place this expansion needs to occur.
jimmylewis
force-pushed
the
defaultDestTemplate
branch
from
ff5e6f5 to
d252a09
Compare
| ILibraryInstallationState result = converter.ConvertToLibraryInstallationState(stateOnDisk); | ||
|
|
||
| Assert.AreEqual("lib/library", result.DestinationPath); | ||
| } |
There was a problem hiding this comment.
Would be great to see some explicit tests of ExpandDestination that showed the 'before' and 'after' as DataRows.
|
This no longer works for LibMan CLI |
3 participants
This resolves #68, except I used [Name] instead of [LibraryName]. It felt like LibraryName should be matched with LibraryVersion and that felt verbose, so I took the shorter versions.
This expansion is applied when we expand the ManifestOnDisk (which is either read from disk or from a raw JSON) into LibraryInstallationState. This is where we determine to use the defaultDestination or a library-specific destination, so it should be the only place this expansion needs to occur.