Skip to content

Move to htmlparser2#64

Merged
eGavr merged 4 commits intomasterfrom
move-to-htmlparser2
Jul 23, 2014
Merged

Move to htmlparser2#64
eGavr merged 4 commits intomasterfrom
move-to-htmlparser2

Conversation

@eGavr
Copy link
Copy Markdown
Member

@eGavr eGavr commented Jul 23, 2014

closes #58

@eGavr
Copy link
Copy Markdown
Member Author

eGavr commented Jul 23, 2014

/cc @tadatuta @andrewblond

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same when pulling a4fa5c1 on move-to-htmlparser2 into 106f180 on master.

Comment thread lib/utils.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Почему у опций поменялись значения?

@eGavr
Copy link
Copy Markdown
Member Author

eGavr commented Jul 23, 2014

Перед ответом сразу скажу, что переход на другой парсер был связан с тем, что в парсере htmlparser нашел пару багов, тулзу перестали поддерживать, поэтому просить их чинить нет смысла, а htmlparser2 гораздо популярнее и активно поддерживается!

charsAroundDiff: 40 - кажется, что такое количество символов удобнее для поиска диффа в основном html

ignoreWhitespace: false - и в htmlparser, и в htmlparser2 именно такое значение стоит по умолчанию, поэтому я и тут поставил его по умолчанию.

@blond
Copy link
Copy Markdown
Member

blond commented Jul 23, 2014

ignoreWhitespace: false - и в htmlparser, и в htmlparser2 именно такое значение стоит по умолчанию, поэтому я и тут поставил его по умолчанию.

Почему раньше было true, это же ломает API и здравый смысл, разве нет? Мы ожидаем, что html с пробелами и без являются эквивалентными, и если не игнорировать пробельные символы, то диффер будет отрабатывать не правильно.

@eGavr
Copy link
Copy Markdown
Member Author

eGavr commented Jul 23, 2014

Ну тут смотря кто и что ожидает, возможно кому-то потребуется обратная ситуация!

@eGavr
Copy link
Copy Markdown
Member Author

eGavr commented Jul 23, 2014

ЛАДНО! Ты прав) надо чтобы было true по умолчанию!

@blond
Copy link
Copy Markdown
Member

blond commented Jul 23, 2014

возможно кому-то потребуется обратная ситуация

Если считаешь, что кому-то нужно сравнивать html с учётом пробельных символов, коментариев и т.д. нужно добавить опций.

Парсер и диффер это не одно и тоже, поэтому полагаться на значение по умолчанию для парсера не очень правильно на мой взгляд. Я считаю, что парсить по умолчанию правильней с учётом пробельных символов, а сравнивать на эквивалентность — без.

В любом случае ломать API это зло, поэтому предлагаю до следующей мажорной версии не менять значения по умолчанию.

@blond
Copy link
Copy Markdown
Member

blond commented Jul 23, 2014

А что на счёт verbose опции?

@eGavr
Copy link
Copy Markdown
Member Author

eGavr commented Jul 23, 2014

У этого парсера htmlparser2 нет этой опции!

Да! согласен! я в скором будущем так и сделаю!
Буду парсить с учетом пробельных символов, а сравнивать на эквивалентность без!

@eGavr
Copy link
Copy Markdown
Member Author

eGavr commented Jul 23, 2014

Полагаю, что этот пул реквест можно вливать, так как задача с переходом на новый парсер решена! а я заведу отдельную задачу, где модифицирую опцию сравнения пробельных символов!

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same when pulling ec27713 on move-to-htmlparser2 into 106f180 on master.

@blond
Copy link
Copy Markdown
Member

blond commented Jul 23, 2014

:squirrel:

eGavr added a commit that referenced this pull request Jul 23, 2014
@eGavr eGavr merged commit 9b91f3d into master Jul 23, 2014
@eGavr eGavr deleted the move-to-htmlparser2 branch July 23, 2014 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

New version of parser

3 participants