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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Regexes for JavaScript objects #1

Closed
baseplate-admin opened this issue Dec 22, 2021 · 26 comments
Closed

Fix Regexes for JavaScript objects #1

baseplate-admin opened this issue Dec 22, 2021 · 26 comments
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed

Comments

@baseplate-admin
Copy link
Owner

Currently the regex can't add comma to JavaScript objects which has more than 2 variables

For example :

{
    hello: 'world',
    foo: 'bar'
};

This will turn into :

{
    hello: 'world',
    foo: 'bar'; // Notice the line break? 馃
};

Now the python regex that i am using is :

NEW_LINE_REPLACE_PATTERN: Pattern = re.compile(
    r"(?!^)(?<![\s{,\"\`;])\n(?!^(\s\S*($|[\"\`)$]|)}))(?![\w\s]*[\)])"
)
@baseplate-admin baseplate-admin added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed wontfix This will not be worked on bug Something isn't working and removed wontfix This will not be worked on labels Dec 22, 2021
@ghost
Copy link

ghost commented Dec 22, 2021

@baseplate-admin I want to this one :)

@baseplate-admin
Copy link
Owner Author

@baseplate-admin I want to this one :)

Hi there. Thanks for replying.

Do you have a working regex? If you do please put a pull request and i will happily merge it. 馃コ

( if you dont understand any part of my code. Please do tell me )

@ghost
Copy link

ghost commented Dec 22, 2021

@baseplate-admin I want to this one :)

Hi there. Thanks for replying.

Do you have a working regex? If you do please put a pull request and i will happily merge it. 馃コ

( if you dont understand any part of my code. Please do tell me )

I am a bit familiar with regex. I work on this for a little bit and If I get stuck I ask you for help. Happy coding :)

@baseplate-admin
Copy link
Owner Author

baseplate-admin commented Dec 22, 2021

I am a bit familiar with regex. I work on this for a little bit and If I get stuck I ask you for help. Happy coding :)

Thank you very much. Since this is one of my very first project i might've not followed the best practices. But thank you again for your contribution. I will be waiting for your pull request . Cheers馃嵒

Edit : I am assigning you for this issue

@ghost
Copy link

ghost commented Dec 22, 2021

I am a bit familiar with regex. I work on this for a little bit and If I get stuck I ask you for help. Happy coding :)

Thank you very much. Since this is one of my very first project i might've not followed the best practices. But thank you again for your contribution. I will be waiting for your pull request . Cheers馃嵒

Edit : I am assigning you for this issue

@baseplate-admin I am pretty sure the issue is with this part of regex (?<![\s{,\"\;])\n because it matches when \n (which is matching the line break) is not proceeded by [\s{,\"\;]

Instead of this we can modify this one (?<=(["']\b))(?:(?=(\\?))\2.)*?(?=\1) to match every word in the double or single quotation marks and add the comma after each match.
Give me your points on this one.

@baseplate-admin
Copy link
Owner Author

baseplate-admin commented Dec 23, 2021

I think i kinda see your point.

match every word in the double or single quotation

But lets take a look here. Copied from here

So here you can see that there are JS specific words like break , try , case. Also there are numbers. So if we try your approach it might not work all the time.

For example your approach won't work on this:

const superCoolNumber = 1
const carsNumbers = 12

Because what we want is ( valid one line JS ) :

const superCoolNumber = 1;const carsNumbers = 12

(?<=(["']\b))(?:(?=(\?))\2.)*?(?=\1)

This regex probably wont work in our case

For example :

const hello = 'world'
const foo = {
    str : 'bar'
}

Will turn into :

const hello = 'world';
const foo = {
    str : 'bar'; // See the problem ?
}

Also the regex should match ` ( backticks ) because they are common in JS apps.

(?<=(["`']\b))(?:(?=(\\?))\2.)*?(?=\1)

Now my approach here is to check if theres a closing bracket } after the newline Character. If there is dont add whitespace.

anime(
    {
        targets: '.animejs__facebook__button',
        translateX: 40 * 2,
        easing: 'easeOutSine',
        duration: 150,
        opacity: 0,
        scale: 0.2 // Dont add a line break here
    } // Capture this bracket right here, if  there is a bracket dont add a line break inside. Instead preferably add a comma
);

But i just cant figure that regex out

@baseplate-admin
Copy link
Owner Author

baseplate-admin commented Dec 23, 2021

Also on another note. Do you think i should add testing? Will it help you in any way?

If you do, could you please tell me what testing lib i should use?

@baseplate-admin baseplate-admin removed the enhancement New feature or request label Dec 23, 2021
@ghost
Copy link

ghost commented Dec 23, 2021

I think i kinda see your point.

match every word in the double or single quotation

But lets take a look here. Copied from here

So here you can see that there are JS specific words like break , try , case. Also there are numbers. So if we try your approach it might not work all the time.

For example your approach won't work on this:

const superCoolNumber = 1
const carsNumbers = 12

Because what we want is ( valid one line JS ) :

const superCoolNumber = 1;const carsNumbers = 12

(?<=(["']\b))(?:(?=(?))\2.)*?(?=\1)

This regex probably wont work in our case

For example :

const hello = 'world'
const foo = {
    str : 'bar'
}

Will turn into :

const hello = 'world';
const foo = {
    str : 'bar'; // See the problem ?
}

Also the regex should match ` ( backticks ) because they are common in JS apps.

(?<=(["`']\b))(?:(?=(\\?))\2.)*?(?=\1)

Now my approach here is to check if theres a closing bracket } after the newline Character. If there is dont add whitespace.

anime(
    {
        targets: '.animejs__facebook__button',
        translateX: 40 * 2,
        easing: 'easeOutSine',
        duration: 150,
        opacity: 0,
        scale: 0.2 // Dont add a line break here
    } // Capture this bracket right here, if  there is a bracket dont add a line break inside. Instead preferably add a comma
);

But i just cant figure that regex out

What about positive and negative lookaheads like these two: (?<=\{) and (?=\}) to identify curly brackets?
(BTW I am still working on this. New ideas will be dropped here for sure.)

@ghost
Copy link

ghost commented Dec 23, 2021

Also on another note. Do you think i should add testing? Will it help you in any way?

If you do, could you please tell me what testing lib i should use?

I think Unit testing is quite important for this project.
I myself am still learning but I've seen its efficiency in every other project on here.

@baseplate-admin
Copy link
Owner Author

baseplate-admin commented Dec 23, 2021

I think Unit testing is quite important for this project. I myself am still learning but I've seen its efficiency in every other project on here.

Thanks for recommendation. So should i use pytest or unittest?

I will work on the testing today :)

@baseplate-admin
Copy link
Owner Author

baseplate-admin commented Dec 23, 2021

What about positive and negative lookaheads like these two: (?<=\{) and (?=\}) to identify curly brackets? (BTW I am still working on this. New ideas will be dropped here for sure.)

Your approach is correct 馃コ. Yea positive lookahead is the right way to go. But you have to ignore the whitespaces before the } ( bracket ). which i can't figure out 馃

But don鈥檛 feel pressured. 馃槃 Take as much time as you need

@ghost
Copy link

ghost commented Dec 23, 2021

I think Unit testing is quite important for this project. I myself am still learning but I've seen its efficiency in every other project on here.

Thanks for recommendation. So should i use pytest or unittest?

I will work on the testing today :)

Currently, I am studying pytest. In this other project I did a little contribution https://github.com/neurodata/hyppo they validate everything with pytest. That could be a suitable choice.

Yeah, I keep working on the regex :) 馃槃

@baseplate-admin
Copy link
Owner Author

baseplate-admin commented Dec 23, 2021

@najmieh Hey do you think i should add this into the test ?

anime(
    {
        targets: '.animejs__facebook__button',
        translateX: 40 * 2,
        easing: 'easeOutSine',
        duration: 150,
        opacity: 0,
        scale: 0.2 // Dont add a line break here
    } // Capture this bracket right here, if  there is a bracket dont add a line break inside. Instead preferably add a comma
);

So that when it finally works we know it works . ( Which means it will always fail unless we finish this error )

In this other project I did a little contribution https://github.com/neurodata/hyppo

Wooh 馃槷 !!! Very cool project 馃

@ghost
Copy link

ghost commented Dec 23, 2021

@najmieh Hey do you think i should add this into the test ?

anime(
    {
        targets: '.animejs__facebook__button',
        translateX: 40 * 2,
        easing: 'easeOutSine',
        duration: 150,
        opacity: 0,
        scale: 0.2 // Dont add a line break here
    } // Capture this bracket right here, if  there is a bracket dont add a line break inside. Instead preferably add a comma
);

So that when it finally works we know it works . ( Which means it will always fail unless we finish this error )

In this other project I did a little contribution https://github.com/neurodata/hyppo

Wooh 馃槷 !!! Very cool project 馃

Yes, Good idea!
Just one question: Need a pattern to identify the white spaces and remove those within this context and variety?

@baseplate-admin
Copy link
Owner Author

baseplate-admin commented Dec 23, 2021

@najmieh So basically yea. We should ignore the whitespace and get the next character after newline. And then merge that into r"(?!^)(?<![\s{,\"\`;])\n(?!^(\s\S*($|[\"\`)$]|)}))(?![\w\s]*[\)])" pattern

anime(
    {
        targets: '.animejs__facebook__button',
        translateX: 40 * 2,
        easing: 'easeOutSine',
        duration: 150,
        opacity: 0,
        scale: 0.2
    } // Ignore the whitespaces and capture the brackets after /n [ newline ]
);

Edit : I will have the tests ready by tomorrow

@baseplate-admin
Copy link
Owner Author

baseplate-admin commented Dec 24, 2021

@najmieh Hey i added the tests.

But the test's EXPECTED_JS may look ugly.

Run it with :

pipenv run test

But for now i am doing a fallback method to keep the extension running :

for i in regex_occurances:
    minified_js = minify_html.minify(
        i,
        minify_js=True,
    )
    replaced_regex_occurances.append(minified_js)

When this issue is closed it will be :

 for i in regex_occurances:
        substituated_regex = sub(NEW_LINE_REPLACE_PATTERN, r";", i)
        replaced_regex_occurances.append(substituated_regex)

@ghost
Copy link

ghost commented Dec 26, 2021

This looks good! I've been out of self-learning for couple days :(
I get back to the issue soon.
Not on hold.

@baseplate-admin
Copy link
Owner Author

baseplate-admin commented Dec 26, 2021

This looks good!

Thank you 馃槃 馃コ

I've been out of self-learning for couple days :(
I get back to the issue soon.
Not on hold.

Take your time 馃榿. No worries 馃槈. In the mean time I will be working on other stuffs 馃槢.

@ghost
Copy link

ghost commented Dec 28, 2021

@baseplate-admin this regex identifies all the white spaces except for the last one! (?=\s)
I need to figure out how to combine that with this (?<=\[) and this (?=\])

@baseplate-admin
Copy link
Owner Author

baseplate-admin commented Dec 28, 2021

@baseplate-admin this regex identifies all the white spaces except for the last one! (?=\s) I need to figure out how to combine that with this (?<=\[) and this (?=\])

Hi there, @najmieh

var x = {
    hello: 'world',
    list: ['python', 'django', 'js']
};

var y = {
    hello:'world',
    list: {
        test: '.py'
    }
}

Your approach wont work on all the above methods.

@ghost
Copy link

ghost commented Dec 28, 2021

Yeah, I see your point. I come up with a better idea. 馃椏馃椏馃椏

@baseplate-admin
Copy link
Owner Author

baseplate-admin commented Dec 28, 2021

@najmieh Thank you for contributing here. Please read this discussion here. If the other library is chosen, you might need to contribute there. Because I think a library by an official django developer would be better than my poorly written library.

I thank you again for working with me. This is my very first open source project and so it meant a lot working with you here.

@ghost
Copy link

ghost commented Dec 28, 2021

yeah, needs a meticulous understanding! I was not aware.
requires a better solution.

@baseplate-admin
Copy link
Owner Author

baseplate-admin commented Dec 28, 2021

requires a better solution.

Yep spending developer time in solving the same issue in 2 different way will produce no result (I am looking at you flask) Instead we should look forward to improving functionality, not reinventing the wheel. Because if we dont focus on improving functionality we will have 10 packages that are not feature complete and they essentially do the same thing.

Lets hear from @adamchainz before taking any step. I worked with him today and he seems a nice guy.

@adamchainz
Copy link
Contributor

I am afraid I'm not interested in attempting to parse JS with a regex. I feel like it's unlikely to be possible. Only "regular" languages can be parsed with a regex, and JavaScript has so many features and quirks I believe it's unlikely to be possible. You need a real parser.

I'm going to unsub from this issue now.

@baseplate-admin
Copy link
Owner Author

baseplate-admin commented Dec 29, 2021

Got it. I am gonna work on rust esbuild. Thanks 馃コ.

Also @najmieh Thank you for spending some time on this hobby project of mine.

@baseplate-admin baseplate-admin unpinned this issue Dec 29, 2021
@baseplate-admin baseplate-admin unassigned ghost Dec 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants