Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DomCrawler] Added Crawler::innerText() method#42338
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| /** | ||
| * Tests that innerText() returns only text that is the direct descendent of the current node, in contrast to | ||
| * text() that returns the text of all child nodes. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The comment is not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A test scenario is not needed, in a technical sense, but it is needed in a maintainability sense. It conveys what the original intent of the test is, which helps greatly in determining how to fix it when it should break in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
From my POV we can keep it, buttestInnerText() method name makes clear what's the point here 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Perhaps, but I think writing test scenarios is a good habit to be into.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we use this anywhere else in the DomCrawler's test suite? If not, I am in favour of removing the comment and, if necessary, change the method name to something liketestInnerTextReturnsContentOfDirectDescendantTextNodesExclusively().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Strongly disagree. Prose is much easier to read as space-separated words instead of stuffing entire sentences into camel-cased method names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
in this casetestInnerText() is enough
| */ | ||
| publicfunctioninnerText():string | ||
| { | ||
| return$this->filterXPath('.//text()')->text(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This can also be expressed aschild::node()/text(). I don't know which is "better".
fabpot commentedAug 26, 2021
@Bilge Can you take suggestions into account? |
Bilge commentedAug 26, 2021
@fabpot Suggestions have been taken into account. |
fabpot commentedSep 21, 2021
Thank you@Bilge. |
Adds a method to get the inner text that is directly descended from the current node only, ignoring text nodes in any child nodes.