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

Add regular expression support #132

Merged
merged 6 commits into from
Mar 15, 2017
Merged

Conversation

fatfisz
Copy link
Contributor

@fatfisz fatfisz commented Mar 1, 2017

This PR adds a support for the regular expressions. The feature was alread requested a few times (e.g. #88 and #127).

This is a proposed implementation and it may and should be a subject to changes if there is a better solution to the problem.


RegExps are defined in the alias part of the plugin options. For an alias to be treated as a RegExp it has to either start with '^' or end with '$'.

RegExp resolving has a higher priority than alias resolving but a lower priority than the root resolving.

I'm convinced that it makes sense to give them the highest priority, but I'm leaving it like that for now - got to catch some sleep ;)

Once a path is matched, it is changed to the value passed in the alias options with those changes:

  • each '\\\\' is changed to '\\'
  • each '\\n' where n is a non-negative integer is changed to the n-th matched group
  • if n in '\\n' is greater than the number of the matched groups then it is changed to '' (an empty string), and not 'undefined'

@codecov
Copy link

codecov bot commented Mar 1, 2017

Codecov Report

Merging #132 into master will not change coverage.
The diff coverage is 100%.

Impacted Files Coverage Δ
src/getRealPath.js 100% <100%> (ø)
src/index.js 100% <100%> (ø)

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 4325ad5...d2857d1. Read the comment docs.

@tleunen
Copy link
Owner

tleunen commented Mar 3, 2017

Nice work again @fatfisz. I'm a little bit busy but I'll try to take a look at the code tomorrow

@tleunen
Copy link
Owner

tleunen commented Mar 4, 2017

I like the fact that the root config has the highest priority.

Forcing the regex to start with ˆ or end with $ looks ok. Should be fine for most people I guess and we'll be able to iterate on this if required.

Nothing else to say, I'll merge it during the weekend unless you want to add something?

@fatfisz
Copy link
Contributor Author

fatfisz commented Mar 6, 2017

Hi, after all I think that RegExps should have the lowest priority, because they are this sort of catch-all mechanism and can handle more cases than aliases. For example:

alias: {
  something: 'path/to/something',
  '^.*$': 'everything/else',
}

Even though an alias for 'something' is declared, it won't ever get resolved, because currently RegExp has bigger priority. I trust that this makes sense, so I'll go ahead and make the change.

Also I first wanted to confirm if the feature is ok and then add the docs - so I'll add them while I'm at that.

@tleunen
Copy link
Owner

tleunen commented Mar 8, 2017

You have a good point here @fatfisz :)
I think the feature makes total sense, but I'm not too sure to understand why there's these 4 \\\

Shouldn't this be done automatically when the system finds it's a regex? I guess it's not that important too since people shouldn't really use a backslash in their path/filename

@fatfisz
Copy link
Contributor Author

fatfisz commented Mar 8, 2017

We are dealing with a string here (not the regexp), and that's where the problems are coming from; let's look at an example:

If we had a string '\1', then in reality it would be '�', because "1" will be a code point of the character. That's why we need to have '\\1' - otherwise we cannot be sure whether it's about the character no. 1 or about the group number.

With that out of the way, let's look at this example: '\\\\1'. After escaping this is '\\1'. The decision to be made here is whether to treat it as an escaped backslash and 1 (so '\1') or a backslash and the first matched group (so '\group'). But in the second case, how would we represent '\1' before double escaping? What should the user provide as an input to get a string '\1'?

Hence the quadruple backslash - first escaping because of the need to discern between '\1' and '\group' and the second because we are dealing with a string.

I hope this is clear, but as usually it might be too long and clear only to me, so thanks for the patience :)


Side note:
In the template-land we could try to use String.raw, but for now \1 is treated as an octal literal. There is a proposal that changes that IIRC.

@fatfisz
Copy link
Contributor Author

fatfisz commented Mar 9, 2017

Btw. by "clear only to me" I meant my explanation and not the thing that I was trying to explain 😄 Basically explaining is not easy for me, as you can see by the length of the previous comment 😜

@fatfisz
Copy link
Contributor Author

fatfisz commented Mar 15, 2017

@tleunen Hi, is there anything I can do to help with this PR?

I understand that life is not only GitHub, no pressure here :)

@tleunen
Copy link
Owner

tleunen commented Mar 15, 2017

Hey @fatfisz, I'm very sorry, I've been very busy these past few weeks with personal things so I can't give the time I'd like to the OSS projects I have to maintain.

Having said that, I really appreciate your contributions, everything looks fine in this PR again. I'll merge it and grant you a collaborator access to this repo.

Before releasing a new version, I'd like to test the latest changes in one of my project (and alongside the eslint-import-resolver-babel-module project to make sure everything's fine.

@tleunen tleunen merged commit 6d87b25 into tleunen:master Mar 15, 2017
@fatfisz
Copy link
Contributor Author

fatfisz commented Mar 15, 2017

Thanks for merging!

Also I hope all is well for you; personal matters are more important than OSS.

@fatfisz fatfisz deleted the regexp-support branch March 29, 2017 18:07
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