JPeer264 / node-rcs-core

Rename css selectors across all files
MIT License
37 stars 7 forks source link

Rework the selector library code so it's not mixing stuff #91

Closed X-Ryl669 closed 5 years ago

X-Ryl669 commented 5 years ago

This is a work in progress and is not intended to be merged as is.

Before I get too deep into refactoring, please review what's being done so far so I can get some feedback.

This is supposed to fix #85 and #51.

The idea here is to let class and id work independently and exactly like the default BaseLibrary. Acting this way, there is no possible confusion between a class or id selector anymore (issue #51) and I'm able to reuse most of the code in BaseLibrary (so now there's less places to have bug 😅). Similarly, for issue #88, I've implemented simple cuckoo hashing. Please note that I'm still using the "global" NameGenerator instance even if it's not optimal, but changing this can be done later, hopefully, without breaking other code, only improving the compression.

I now have to fix all other code making use of SelectorLibrary to instead use 2 instances (ClassSelectorLibrary and IdSelectorLibrary).

JPeer264 commented 5 years ago

This looks very promising. I like the new way of having different files for ids and classes. Btw I think removing options.extend can be done. I already thought about removing it once, but always kept it.

I have not much feedback here, it's very clean actually. Also thanks a lot for your contribution :+1:

X-Ryl669 commented 5 years ago

All tests except (obviously) SelectorLibrary now pass. I need to write a test for SelectorsLibrary soon. #51 should be fixed too.

The changes are mainly due to the fact that IdSelector goes first then ClassSelector and since they all share the same NameGenerator instance, the mapping is changed to reflect this. So, instead of getting .a {} .b {} #c we are getting .b {} .c {} #a

Also, I've modified the API to add a getAllRegex method instead of getAll since it's not called anywhere without {regex: true} (well, at least so far).

JPeer264 commented 5 years ago

Awesome.

Regarding the nameGenerator. I think it's gonna be fixed if #64 is getting merged. You think that would be better if we merge #64 first, so you have an own nameGenerator instance per class?

coveralls commented 5 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling b5c103ab98aa678b769a09a167b080417684599d on X-Ryl669:OptimalReplace into 55ff9746c1ea58ce3ede8e5ebac53417bd3553ee on JPeer264:master.

X-Ryl669 commented 5 years ago

I don't think so. #64 is already very clean but conflicts with the current master, I would prefer getting this one first and then merge it later on.

Right now I think the status should be OK for review before merging. I've added all tests I could think of.

To get the optimal coverage factor, I had to call 3 of the AttributeLibrary pseudo abstract methods. I don't know if it's the correct way to do, yet it works.

JPeer264 commented 5 years ago

Thanks a lot for contributing 👍