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.

Sign up for GitHub

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

Jump to bottom

[FEATURE] Add HtmlPruner::removeRedundantClasses #708

Merged
merged 6 commits into from
Sep 27, 2019
Merged

[FEATURE] Add HtmlPruner::removeRedundantClasses #708

merged 6 commits into from
Sep 27, 2019

Conversation

JakeQZ
Copy link
Contributor

@JakeQZ JakeQZ commented Sep 13, 2019

Part of #380.

@JakeQZ JakeQZ added this to the 3.0.0 milestone Sep 13, 2019
@JakeQZ JakeQZ self-assigned this Sep 13, 2019
oliverklee
oliverklee requested changes Sep 13, 2019
View reviewed changes
@JakeQZ
Copy link
Contributor Author

JakeQZ commented Sep 14, 2019

I've just replied to the comments for now. I'll look at committing some changes tomorrow... 🌙

JakeQZ added a commit that referenced this pull request Sep 14, 2019
@JakeQZ
[FEATURE] Add an ArrayIntersector class
68b1243 
Part of #708.
@JakeQZ JakeQZ mentioned this pull request Sep 14, 2019
Merged
@JakeQZ
Copy link
Contributor Author

JakeQZ commented Sep 14, 2019

I find this hard to grok with the multiple array_flips. Maybe we can wrap this in a well-named method to reduce the level of detail required to understand this?

Or perhaps a simple class could be provided to abstract the detail

On the basis of the adage “if a job’s worth doing, it’s worth doing well”, I’ve added such a class. I dropped the prefix Batch… as that may imply a method that does the batching, whereas in fact it has a method intended to be called repeatedly as part of some batch processing. See #710.

oliverklee pushed a commit that referenced this pull request Sep 14, 2019
@JakeQZ @oliverklee
[FEATURE] Add an ArrayIntersector class (#710)
716ce99 
Part of #708.
@JakeQZ
Copy link
Contributor Author

JakeQZ commented Sep 16, 2019

I've made changes based on the review suggestions and repushed.

@JakeQZ
Copy link
Contributor Author

JakeQZ commented Sep 17, 2019

I've made changes based on the review suggestions and repushed.

In this change I introduced these private methods:

private function removeClassesFromNodes(\DOMNodeList $nodeList, array $classesToKeep);
private function removeClassAttributeFromNodes(\DOMNodeList $nodeList);

I chose …Nodes in the method name because the parameter is a DOMNodeList, but now I think about it, the methods should probably be named …Elements because they require that the nodes are elements (only elements can have attributes). And perhaps then the parameter should be $elements rather than $nodeList, and the local variables renamed …node… to …element…...?

(We also have various instances in the code base of private methods and local variables using ‘node’ in the name, where they are necessarily elements, and perhaps should consider changing the names for clarity, but I’d say not for 3.0.0 – except #717.)

@oliverklee
Copy link
Contributor

but now I think about it, the methods should probably be named …Elements because they require that the nodes are elements (only elements can have attributes). And perhaps then the parameter should be $elements rather than $nodeList, and the local variables renamed …node… to …element…...?

Yes, this makes sense and sounds good.

@JakeQZ
Copy link
Contributor Author

JakeQZ commented Sep 17, 2019

the methods should probably be named …Elements … the parameter should be $elements rather than $nodeList, and the local variables renamed …node… to …element…...?

Yes, this makes sense and sounds good.

I've renamed accordingly and repushed.

@JakeQZ JakeQZ mentioned this pull request Sep 25, 2019
Merged
oliverklee
oliverklee requested changes Sep 26, 2019
View reviewed changes
Copy link
Contributor

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks generally very good. Just some minor remarks. And could you please rebase?

@JakeQZ
Copy link
Contributor Author

JakeQZ commented Sep 27, 2019

Just some minor remarks.

Have made suggested changes.

And could you please rebase?

Well, I used git rebase master and it seems to have gone somewhat awry. Initially I had not done a git pull on master so it was not up to date. But after doing so, it kept trying to merge the initial changes from this PR on top of the later changes. When finally all sorted, I see that the file diff now shows all of the other changes from master since this PR was started - exactly what we were trying to avoid!

I've saved the command prompt transcript which I can post to identify what I might have done wrong...

@oliverklee
Copy link
Contributor

Well, I used git rebase master and it seems to have gone somewhat awry

I'll do my best to help.

@oliverklee
Copy link
Contributor

Some ideas to resolve the git problem:

  1. change to master and pull (just to be on the safe side)
  2. drop the merge commits from the branch (using git rebase -i HEAD~25
  3. rebase from master again and resolve any rebase conflicts in the individual commits

@oliverklee
Copy link
Contributor

@JakeQZ And if you like, I can also resolve the problem on my machine, force-push and then provide the steps I executed.

@JakeQZ
[FEATURE] Add HtmlPruner::removeRedundantClasses
d7d7b16 
Part of #380.
@JakeQZ
Changes suggested in code review:
25e9b2e 
- Add Jake Hotson as author of `HtmlPruner` and `HtmlPrunerTest`;
- Remove redundant “list of” from PHPDoc for `removeRedundantClasses()`;
- Move branches of `removeRedundantClasses()` into separate methods;
- Allow parameter of `removeRedundantClasses()` to have default value;
- Add comment clarifying alternate branch in `removeRedundantClasses()` is
  merely an optimization;
- Use `ArrayIntersector` to abstract the array intersection with ‘flipped’
  arrays;
- Add multiple classes case to `classesToKeepDataProvider()`;
- Change ‘html’ to ‘HTML’ in array keys for human consumption;
- Split `removeRedundantClassesRemovesOnlyClassesNotToKeep()` into two separate
  tests;
- Refactor test data providers to more cleanly provide the appropriate data for
  each test.

Also added missing preposition to sentence in PHPDoc for `ArrayIntersector`.
@JakeQZ
Renamed ‘node’ to ‘element’ in applicable method/variable names.
9de2bf1
@JakeQZ
Made data provider names more succinct.
60a232f
@JakeQZ
Changes suggested in 2nd code review:
b4b18fd 
- “class names” -> “names of classes” in PHPDoc;
- Added specialized assertion methods: `assertSubstringCount` and
  `assertMinified`.
@JakeQZ JakeQZ force-pushed the feature/remove-redundant-classes branch from a39cc86 to b4b18fd Compare September 27, 2019 13:43
@oliverklee
Copy link
Contributor

oliverklee commented Sep 27, 2019 •
edited
Loading

Should this PR close #380 when merged?

@JakeQZ
Copy link
Contributor Author

JakeQZ commented Sep 27, 2019 •
edited
Loading 

2. drop the merge commits from the branch (using `git rebase -i HEAD~25` 
3. rebase from master again and resolve any rebase conflicts in the individual commits

I think I've sorted it. It seems that after git rebase master, the local copy of the branch must be --force pushed. If an ordinary push is attempted, you're told you need to do a pull first which then ends up with the entire set of commits on the branch being duplicated (or triplicated), and the 'patching base' not being moved at all. (I removed the duplicated commits using git rebase -i HEAD~25.)

@JakeQZ
Copy link
Contributor Author

JakeQZ commented Sep 27, 2019

Should this close #380 when merged?

Not yet - there's the final method which takes a CssInliner parameter still to do...

@oliverklee
Copy link
Contributor

If an ordinary push is attempted, you're told you need to do a pull first which then ends up with the entire set of commits on the branch being duplicated (or triplicated), and the 'patching base' not being moved at all.

Oh, yes, that message. The action the message recommends is completely bogus. Just last week, I told exactly that to the attendees at a Git workshop. :-)

oliverklee
oliverklee approved these changes Sep 27, 2019
View reviewed changes
Copy link
Contributor

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful!

@oliverklee oliverklee merged commit 18b2f1f into master Sep 27, 2019
@oliverklee oliverklee deleted the feature/remove-redundant-classes branch September 27, 2019 17:15
This was referenced Sep 27, 2019
Merged
Merged
@JakeQZ JakeQZ mentioned this pull request Oct 6, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliverklee oliverklee oliverklee approved these changes

@zoliszabo zoliszabo Awaiting requested review from zoliszabo

Assignees

@JakeQZ JakeQZ

Labels
enhancement
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@JakeQZ @oliverklee