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

Hotfix for module named like "is-function" #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

leoj3n
Copy link
Contributor

@leoj3n leoj3n commented Jul 27, 2017

New "function" regex:

https://regex101.com/r/jL1iW6/8/tests

const regex = /(?:[^\-]|^)function/gm;
const str = `is-function`;
let m;

while ((m = regex.exec(str)) !== null) {
    // This is necessary to avoid infinite loops with zero-width matches
    if (m.index === regex.lastIndex) {
        regex.lastIndex++;
    }
    
    // The result can be accessed through the `m`-variable.
    m.forEach((match, groupIndex) => {
        console.log(`Found match, group ${groupIndex}: ${match}`);
    });
}

Without this, "can-util/js/is-function/is-function" stops short at "can-util/js/is-function":

image

With new (?:[^\-]|^)function regex:

image

@justinbmeyer Is there a better solution? Feels like a temporary fix; but best I could come up with.

Without this, "can-util/js/is-function/is-function" stops short at "can-util/js/is-function"
@leoj3n
Copy link
Contributor Author

leoj3n commented Jul 27, 2017

Will fix the broken [can-util/js/is-function/is-function] link:

image

On that page -> https://canjs.com/doc/can-util.html

@chasenlehara
Copy link

@leoj3n, could you please add a test for this change?

@leoj3n
Copy link
Contributor Author

leoj3n commented Jul 29, 2017

@chasenlehara Seems my new regex pattern is causing an existing test to fail because of how it includes one character behind so where a name like documentjs/tags/function used to make nameChildren[0].token be function, with this new pattern it makes the token be /function.

So, this assertion is failing:

https://github.com/bit-docs/bit-docs-type-annotate/blob/hotfix-for-is-function/lib/typeNameDescription_test.js#L11

Because this if statement is now matching /function instead of function:

https://github.com/bit-docs/bit-docs-type-annotate/blob/hotfix-for-is-function/lib/typeNameDescription.js#L28-L31

I think that if statement adds back function if it got tokenized, or something like that. I think it's maybe a bit of a hack or something. I'm still trying to understand what "tokens" represent and how they are supposed to be used in bit-docs, and why it's necessary to add back function etc.

I had to write the regex the way I did because JavaScript regex doesn't support look behind.

Not sure if the token now being like /function is technically wrong? Probably; but like I said, I still don't yet fully understand what the "token" property in nameChildren[0].token is supposed to represent and be used for (still trying to figure that out). The "token" could be preceded by any character, depending on the source string. So, in this case it is /function, but it could also be .function, or if the source string was myfunction, it would be yfunction, etc...

These reasons are why in the OP that I said I wasn't so sure about the proper way to resolve the issue of having a "name" string with "function" in it like is-function, because I don't fully understand this part of the annotation module for bit-docs.

@leoj3n
Copy link
Contributor Author

leoj3n commented Jul 29, 2017

Hm, it seems we could maybe piggy-back off this hack:

bitovi/documentjs#209

Which means users will have to use \\ to break up the word "function" in a name, like:

@module {function} can-util/js/is-f\\unction/is-f\\unction is-function

@leoj3n
Copy link
Contributor Author

leoj3n commented Jul 31, 2017

Update: Eventually this will be resolved by https://github.com/canjs/can-parse

For now, ignore the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants