-
Notifications
You must be signed in to change notification settings - Fork 180
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
feat: adds glob support for dir
and ignorePattern
#1274
Conversation
looks like you were working from your old main branch 🙂 |
Signed-off-by: Benjamin Kroeger <[email protected]>
d14c344
to
42f98e6
Compare
Set to draft for now, because some failing tests I gave a really simple first review, but I need to have a look into details, but assume I wont have time before upcoming Saturday, because I will be on a conference But it looks really promising and potentially even like a better solution that the other strategy ❤️ |
Signed-off-by: Benjamin Kroeger <[email protected]>
Signed-off-by: Benjamin Kroeger <[email protected]>
because the previous implementation used that only on the migration file's basename (which also caused two files with same name but different extensions to be treated as equal priority) Signed-off-by: Benjamin Kroeger <[email protected]>
This reverts commit 3adc21a.
Signed-off-by: Benjamin Kroeger <[email protected]>
Co-authored-by: Shinigami <[email protected]>
Signed-off-by: Benjamin Kroeger <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I had an in depth look and really like the PR 👍
As you already wrote in the ?
comments, we are free to change the name or inner structure of these function. Theoretically users could import them because it is exported in package.json
, but I would count this more as a legacy backward compatibility and not official support. So I'm free to break even in a minor version bump.
But please you now need to change the both sections of ! and ?
comments so they are solved in this PR, before getting merged.
And if you like, you can also directly use the options destructure pattern in this PR, or I could do this in a separate PR later on.
also updates code documentation Signed-off-by: Benjamin Kroeger <[email protected]>
It may not matter so much in this case, but I recently had to implement some globbing behaviour and found fdir (https://www.npmjs.com/package/fdir) to be much more performant. Should be a simple swap. |
... Lets stay with glob for now, I plan anyways to use |
if you're going with pure js implementations, there is no faster lib that adheres the de-facto-spec as good as this one. I don't think that performance is that much of the essence here either. |
Signed-off-by: Benjamin Kroeger <[email protected]>
Might release next few days, I also currently await feedback of #1262 |
adds support to work with glob patterns for
dir
andignorePattern
as alternative to the current implementation (which usesdir
as relative path tocwd()
and generates a regex fromignorePattern
.CLI requires to additionally set the flag
--use-glob
while theRunnerOptions
for API have been extended with auseGlob
boolean property.When using glob, migration files are sorted by their absolute path (via String.localeCompare) while setting
numeric: true
in the Intl.Collator().The original sorting has not changed (still attempts to extract Date from the filename (either
utc
ortimestamp
) and sorts by the date's numeric value if possible -localeCompare
otherwise)This also fixes the most parts (if not all) of #932 and thus makes #1226 obsolete
Seems to make #911 obsolete as well