5 ms·

+1 to make nicer APIs! It is always good to have more high-quality API designs to look at. That said, it looks more like an API experiment, not a practical sol

+1 to make nicer APIs! It is always good to have more high-quality API designs to look at. That said, it looks more like an API experiment, not a practical solution for a day job, at least in its current state: * response body encoding detection is wrong, as it doesn't take meta tags or BOM into account; * base url detection is wrong, as it doesn't take <base> tag in account; * URL parsing (joining, etc.) is implemented using string operations instead of stdlib, so a careful inspection is required to make sure it works in edge cases. For example, I can see right away that .absolute_links is wrong for protocol-related urls (e.g. "//ajax.microsoft.com/ajax/jquery/jquery-1.3.2.min.js") * html2text used for .markdown is GPL - I know people have different opinion on this, but in my book if you import from a GPL package, your package becomes GPL as well; * each .xpath call parses a chnk of HTML again, even if a tree is already present * shortcuts are opinionated and with no clear behavior, e.g. .links deduplicates URLs by default, it deduplicates them using string matches (so e.g. different order of GET arguments => URLs are considering unique); it checks for '.startswith('#")' which looks arbitrary (what if these links are used in a headless browser? what if someone wants to fetch them using _escaped_fragment which many sites still support? why filter out such URLs if they are relative, but not if they are absolute?)

EmilStenstrom 9y ago
Would you mind posting these as issues?
- kmike84 9y ago
  TBH I don't see myself using this package: in its current stage it is very little code, and almost every method has an issue either with edge cases or with API; also, it is tied to requests library, unnecessarily IMHO, and in my opinion it is GPL even if setup.py says it is MIT. Because there is nothing usable code-wise in requests-html from my point of view (it is no better than existing alternatives), I don't feel like raising these issues, advocating for fixing them, discussing alternative solutions with a goal of improving requests-html. Of course, everyone is free to raise these issues in a repo. I appreciate the work put into requests-html API design, the design is very nice overall. This might be a way to go: create a nice API design, attract people, fix implementation over time, but this battle is not mine, sorry :(
  - kenneth_reitz 9y ago
    GPL dependency removed. All of these improvements I'd like to be made to the software. It's all about getting a nice API in place first, then making it perfect second.
    - kenneth_reitz 9y ago
      I addressed most of your issues, like not using urlparse in the latest release. With libraries like these, it's all about getting the API right first, them optimizing for perfection second. :)
      - kenneth_reitz 9y ago
        <base> tag is now implemented as well. Thanks for bringing that to my attention — I wasn't aware of it!
      - kmike84 9y ago
        :thumbs up: A second iteration of review: * encoding detection from <meta> tags doesn't normalize encodings - Python doesn't use the same names as HTML; * I'm still not sure encoding detection is correct, as it is unclear what are priorities in the current implementation. It should be 1) Content-Type header; 2) BOM marks; 3) encoding in meta tags (or xml declared encoding if you support it); 4) content-based guessing - chardet, etc., or just a default value. I.e. encoding in meta should have less priority than Content-Type header, but more priority than chardet, and if I understand it properly, response.text is decoded both using Content-Type header and chardet. * lxml's fromstring handles XML (XHTML) encoding declarations, and it may fail in case of unicode data (http://lxml.de/parsing.html#python-unicode-strings http://lxml.de/parsing.html#python-unicode-strings), so passing response.text to fromstring is not good. At the same time, relying on lxml to detect encoding is not enough, as http headers should have a higher priority. In parsel we're re-encoding text to utf8, and forcing utf8 parser for lxml to solve it: https://github.com/scrapy/parsel/blob/f6103c8808170546ecf046b9f4ea6dead94de189/parsel/selector.py#L38 https://github.com/scrapy/parsel/blob/f6103c8808170546ecf046.... * when extracting links, it is not enough to use raw @href attribute values, as they are allowed to have leading and trailing whitespaces (see https://github.com/scrapy/w3lib/blob/c1a030582ec30423c40215fcd159bc951c851ed7/w3lib/html.py#L325 https://github.com/scrapy/w3lib/blob/c1a030582ec30423c40215f...) * absolute_links doesn't look correct for base urls which contain path. It also may have issues with urls like tel:1122333, or mailto:. For encoding detection we're using https://github.com/scrapy/w3lib/blob/c1a030582ec30423c40215fcd159bc951c851ed7/w3lib/encoding.py#L187 https://github.com/scrapy/w3lib/blob/c1a030582ec30423c40215f... in Scrapy. It works well overall; its weakness is that it doesn't require a HTML tree, and doesn't parse it, extracting meta information only from first 4Kb using a regex (4Kb limit is not good). Other than that, it does all the right things AFAIK.

EmilStenstrom 9y ago

Would you mind posting these as issues?

kmike84 9y ago
TBH I don't see myself using this package: in its current stage it is very little code, and almost every method has an issue either with edge cases or with API; also, it is tied to requests library, unnecessarily IMHO, and in my opinion it is GPL even if setup.py says it is MIT. Because there is nothing usable code-wise in requests-html from my point of view (it is no better than existing alternatives), I don't feel like raising these issues, advocating for fixing them, discussing alternative solutions with a goal of improving requests-html. Of course, everyone is free to raise these issues in a repo. I appreciate the work put into requests-html API design, the design is very nice overall. This might be a way to go: create a nice API design, attract people, fix implementation over time, but this battle is not mine, sorry :(
- kenneth_reitz 9y ago
  GPL dependency removed. All of these improvements I'd like to be made to the software. It's all about getting a nice API in place first, then making it perfect second.
  - kenneth_reitz 9y ago
    I addressed most of your issues, like not using urlparse in the latest release. With libraries like these, it's all about getting the API right first, them optimizing for perfection second. :)
    - kenneth_reitz 9y ago
      <base> tag is now implemented as well. Thanks for bringing that to my attention — I wasn't aware of it!
    - kmike84 9y ago
      :thumbs up: A second iteration of review: * encoding detection from <meta> tags doesn't normalize encodings - Python doesn't use the same names as HTML; * I'm still not sure encoding detection is correct, as it is unclear what are priorities in the current implementation. It should be 1) Content-Type header; 2) BOM marks; 3) encoding in meta tags (or xml declared encoding if you support it); 4) content-based guessing - chardet, etc., or just a default value. I.e. encoding in meta should have less priority than Content-Type header, but more priority than chardet, and if I understand it properly, response.text is decoded both using Content-Type header and chardet. * lxml's fromstring handles XML (XHTML) encoding declarations, and it may fail in case of unicode data (http://lxml.de/parsing.html#python-unicode-strings http://lxml.de/parsing.html#python-unicode-strings), so passing response.text to fromstring is not good. At the same time, relying on lxml to detect encoding is not enough, as http headers should have a higher priority. In parsel we're re-encoding text to utf8, and forcing utf8 parser for lxml to solve it: https://github.com/scrapy/parsel/blob/f6103c8808170546ecf046b9f4ea6dead94de189/parsel/selector.py#L38 https://github.com/scrapy/parsel/blob/f6103c8808170546ecf046.... * when extracting links, it is not enough to use raw @href attribute values, as they are allowed to have leading and trailing whitespaces (see https://github.com/scrapy/w3lib/blob/c1a030582ec30423c40215fcd159bc951c851ed7/w3lib/html.py#L325 https://github.com/scrapy/w3lib/blob/c1a030582ec30423c40215f...) * absolute_links doesn't look correct for base urls which contain path. It also may have issues with urls like tel:1122333, or mailto:. For encoding detection we're using https://github.com/scrapy/w3lib/blob/c1a030582ec30423c40215fcd159bc951c851ed7/w3lib/encoding.py#L187 https://github.com/scrapy/w3lib/blob/c1a030582ec30423c40215f... in Scrapy. It works well overall; its weakness is that it doesn't require a HTML tree, and doesn't parse it, extracting meta information only from first 4Kb using a regex (4Kb limit is not good). Other than that, it does all the right things AFAIK.