Add incrementer option
#8
Conversation
See https://github.com/sindresorhus/typescript-definition-style-guide |
|
@sindresorhus could you update typescript-definition, not sure how to do it |
index.d.ts
Outdated
| declare namespace unusedFilename { | ||
| interface Options { | ||
| /** | ||
| Filename increase function |
There was a problem hiding this comment.
This should better explain what it does and some use-cases for it. Also add a code example. Same in the readme.
There was a problem hiding this comment.
I have added some text, not too many experiences on docs
| }, | ||
| result: 'fixtures/rainbow - copy - copy - copy.txt' | ||
| } | ||
| ].map(incrementerTest) |
There was a problem hiding this comment.
Instead of mapping here, use test macros: https://github.com/avajs/ava/blob/master/docs/01-writing-tests.md#reusing-test-logic-through-macros
There was a problem hiding this comment.
separate test?
√ sync
√ `incrementer` should be a function
√ `incrementer` should return a string
√ `incrementer` should return unique names
√ custom incrementer
√ async
|
|
||
| const checkIsTried = newFilePath => { | ||
| if (triedFileNames.has(newFilePath)) { | ||
| throw new Error('`incrementer` should return unique names'); |
There was a problem hiding this comment.
I don't know, should it?
If I'm generating a random 4-characters name (like U1HA), but it's already taken and my incrementer generates another random 4-characters name it may happen that it generates U1HA again, I don't think the app should crash in this scenario.
Also maybe a name we tried earlier on it's free to use now, I don't think this check is necessary.
Maybe a maxTries option should be added instead, so if for some reason there are no available names the library throws an error after maxTries number of tries rather than being busy forever.
There was a problem hiding this comment.
I agree we should have a maxTries, but I am not sure about the default value. Infinity or a specific number?
I think unique name check is necessary too.
What do you think? @sindresorhus
There was a problem hiding this comment.
I agree, it should not throw. As for maxTries, I cannot think of a sensible default, so I would go with Infinity (Unless @fabiospampinato can argument for something better).
There was a problem hiding this comment.
I'm setting it to 128, but it's just a number I pulled out of nowhere, my reasoning against Infinity was that if it's impossible to find an unused path for whatever reason I don't want the app to hang forever, you don't want it to give up trying too early either though, so I'm setting it to 128 by default which feels about right to me.
|
Should this option named |
|
I have another idea, how about Generator support? {
* incrementer(filename, extension) {
let i = 0
while(true) {
yield `${filename}(${++i})${extension}`
}
}
}useful case, If I hava a few options, without Generator {
incrementer(filename, extension, counter) {
return ['a.js','b.js','c.js'][counter - 1]
}
}with Generator {
* incrementer() {
yield 'a.js'
yield 'b.js'
yield 'c.js'
}
} |
|
|
||
| Type: `function` | ||
|
|
||
| filename increase function. |
There was a problem hiding this comment.
The readme should be in sync with index.d.ts
|
|
||
| @param filePath - The path to check for filename collision. | ||
| @returns Either the original `filename` or the `filename` appended with a number. | ||
| @returns Either the original `filename` or the `filename` increment by `option.incrementer`. |
There was a problem hiding this comment.
| @returns Either the original `filename` or the `filename` increment by `option.incrementer`. | |
| @returns Either the original `filename` or the `filename` appended with a number (or modified by `option.incrementer` if specified). |
|
|
||
| @param filename - The filename of the file path. | ||
| @param extension - The extension of the file path. | ||
| @param counter - Tried count. |
There was a problem hiding this comment.
Tried count is not really a good description.
|
|
I like the use of generator for this. |
|
totally forgot this PR... |
|
Bump :) |
|
Sorry for the late response, but I'll be busy for a while, no time to work on this. |
TODO:
update
index.d.tstests fail on windows, because of different path separator
check
incrementertype and result to avoid an infinite loopFixes #7