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

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

Merged
merged 10 commits into from Aug 15, 2019

Conversation

@X-Ryl669
Copy link
Contributor

commented Aug 6, 2019

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).

X-Ryl669 added some commits Jul 16, 2019

Add support for AttributeLibrary.
The idea is that selectors will be split between class and id selectors (one instance per type)
That way, we can reuse the BaseLibrary's members for mapping and remove all common code found in SelectorLibrary
(that'll be removed in a later patch).
I've added the required hooks for selector specific code.
Specialization for class and id selectors.
Yes, that's really simple since most work is done in AttributeLibrary.
@JPeer264

This comment has been minimized.

Copy link
Owner

commented Aug 6, 2019

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 馃憤

@X-Ryl669

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

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

This comment has been minimized.

Copy link
Owner

commented Aug 10, 2019

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

This comment has been minimized.

Copy link

commented Aug 11, 2019

Coverage Status

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

@X-Ryl669 X-Ryl669 force-pushed the X-Ryl669:OptimalReplace branch from 1f9f6a5 to 4b5ccbe Aug 11, 2019

@X-Ryl669

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

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
Copy link
Owner

left a comment

Awesome changes. Just two things. After that I think it is ready to merge

// Second: The regular expression operator (* in previous example, '' when none)
// Third: The term used ('selector' in the previous example)
// eslint-disable-next-line class-methods-use-this
getAtttributeSelectorRegex() {

This comment has been minimized.

Copy link
@JPeer264

JPeer264 Aug 13, 2019

Owner

One t too much. Please name it getAttributeSelectorRegex

@@ -51,7 +50,9 @@ const replaceJs = (code, espreeOptions) => {
const raw = node.value.raw.replace(allRegex.templateSelectors, function (txt, p1, p2, p3) {
// p3 contains the content of the class=' or id=", so let's replace them
const newValue = ` ${p3} `;
const replacedAttr = newValue.replace(newValue, match => replaceString(match, regex, ' ', { countStats: false }));
const replacedAttr = p1 === 'class' ?

This comment has been minimized.

Copy link
@JPeer264

JPeer264 Aug 13, 2019

Owner

What about following for readability:

const selectorLib = p1 === 'class' ? selectorsLibrary.getClassSelector() : selectorsLibrary.getIdSelector();
const replacedAttr = newValue.replace(newValue, match => replaceString(match, selectorLib.getAll({ regex: true }), ' ', { countStats: false }));

This comment has been minimized.

Copy link
@X-Ryl669

X-Ryl669 Aug 13, 2019

Author Contributor

Sure, will do tonight.

This comment has been minimized.

Copy link
@X-Ryl669

X-Ryl669 Aug 13, 2019

Author Contributor

It's done. I'll now work on the name generator fix and then on rename-css-selector project to use the new API.

I was planning to change the format of the mapping file so it's hierarchical instead of a plain dictionnary: one per BaseLibrary instance

Instead of:

{
"more": "a",
[...]
}

have:

{
"keyframes": { "stop": "a", [...] },
"class": { "more": "a", [...] },
"id": { "id": "a", [...] },
"variables": { "red": "a", [...] }
}

I'll need to bump the major version, since the API changed.

@X-Ryl669 X-Ryl669 referenced this pull request Aug 14, 2019

@JPeer264 JPeer264 merged commit 48d89bb into JPeer264:master Aug 15, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@JPeer264

This comment has been minimized.

Copy link
Owner

commented Aug 15, 2019

Thanks a lot for contributing 馃憤

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.