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

feat: add a transform method #122

Closed
wants to merge 5 commits into from
Closed

feat: add a transform method #122

wants to merge 5 commits into from

Conversation

pralkarz
Copy link

@pralkarz pralkarz commented Sep 27, 2024

Resolves #107.

The issue and the code say everything, really. 😄

Copy link
Contributor

@SuperchupuDev SuperchupuDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as per the discord conversation, can you add a transform argument to the filter function? or some other way of being able to transform before filtering at all

@pralkarz
Copy link
Author

as per the discord conversation, can you add a transform argument to the filter function? or some other way of being able to transform before filtering at all

For simplicity's sake, I think supporting just one or the other is better. Based on your use cases transforming before the filtering makes more sense. Pushed the change and clarified that in the documentation.

paths.push((directoryPath || ".").substring(root.length));
return function (directoryPath, paths, _filters, transformer) {
const relativePath = (directoryPath || ".").substring(root.length);
paths.push(transformer?.(relativePath, true) ?? relativePath);
Copy link
Contributor

@SuperchupuDev SuperchupuDev Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional chaining and ?? would sadly break compatibility with anything below node 14

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh my, got used to those features so much at work that I forgot they weren't supported at one point. Gonna change later.

Copy link
Contributor

@SuperchupuDev SuperchupuDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also pending the second argument change to a Dirent, just leaving it here as a reminder, amazing work so far :-)

@thecodrr
Copy link
Owner

Hey, I really appreciate the effort but as I mentioned in #107, fdir isn't the right place for this.

P.S. please discuss before implementing any feature to avoid situations like these.

@thecodrr thecodrr closed this Sep 27, 2024
@SuperchupuDev
Copy link
Contributor

SuperchupuDev commented Sep 27, 2024

is there any place we can contact you outside github to discuss fdir related stuff? we've been discussing our fdir prs/issues in the e18e discord server which you might be interested in joining

@thecodrr
Copy link
Owner

I think the discussion should happen under a GitHub issue as that keeps things more localized and easier to reference later on.

With that said, let me know if you prefer something more...informal. I can create a discord server for fdir but to be honest, I don't think the world needs yet another discord server. There are already too many. 😂

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.

[Feature] middleware / custom transforms support
3 participants