Task0 #4
Task0 #4
Conversation
| ] | ||
|
|
||
| def __proxy_request__(self): | ||
| con = HTTPSConnection('news.ycombinator.com') |
There was a problem hiding this comment.
Здесь и далее возможно стоило вынести это в инициализацию класса и вообще во внешнюю переменную.
There was a problem hiding this comment.
Note: this enforcement is applied by the HTTPConnection class. The
HTTPResponse class does not enforce this state machine, which
implies sophisticated clients may accelerate the request/response
pipeline. Caution should be taken, though: accelerating the states
beyond the above pattern may imply knowledge of the server's
connection-close behavior for certain requests. For example, it
is impossible to tell whether the server will close the connection
UNTIL the response headers have been read; this means that further
requests cannot be placed into the pipeline until it is known that
the server will NOT be closing the connection.
Поясню: HTTPSConnection умеет быть переиспользованной(Если tcp-соединение будет закрыто, откроет его заново, если открыто сразу отправит запрос). И это может повысить скорость, но так же вызвать проблемы(зависит от сервера, он может внезапно закрыть соединение). Помимо это повод усложнить код. do_GET do_POST вполне себе простые синхронные методы. Но зато выполняются ThreadingServer. Если же сделать HTTPSConnection статичным свойством(это будет правильней чем глобальный объект) то будем решать вопрос с конкурентным доступом к нему(скорее всего проблем не будет, из-за GIL, но это всё равно вопрос к мультипоточности). И скорее всего ещё и в скорости потеряем, потому что не сможем слать параллельные запросы.
| __regexp_domain__ = re.compile(r'([\w-]+(?:\.[\w-]+)+)') | ||
| __regexp_dash_underline__ = re.compile(r'(-_+|_+-)') | ||
| __regexp_6letter_word__ = re.compile(r'(?i)(?<![\w-])([a-z][\w-]{4}[a-z0-9])(?![\w-])') | ||
|
|
There was a problem hiding this comment.
Решение с регэкспами больше похоже на магию, в которую проще поверить, чем проверить. )
Но это мой минус, я просто не знаю, как пользоваться регами, возможно тут всё очевидно.
There was a problem hiding this comment.
Вроде всё ок.
Я не совсем в курсе, как принято у "взрослых", но может быть стоило помимо модульных тестов ещё комплексный тест с простой частью страницы, чтобы проверить, что несколько последовательных частей программы в совокупности не портят итоговый результат?
| """Split text with rexexp's recursively""" | ||
|
|
||
| # Split by any url-like sequence | ||
| # Example: anyprotocol://yaras.ru/asddas?asd=1&asd=dsa |
There was a problem hiding this comment.
Коммент с примером есть, но по нему непонятно, что произойдёт после выполнения кода.
| # Example: anyprotocol://yaras.ru/asddas?asd=1&asd=dsa | ||
| splited1 = enumerate(__regexp_url__.split(text)) | ||
| for i, t1 in splited1: | ||
| if i % 2 == 0: |
There was a problem hiding this comment.
Возможно стоило указать, почему чётные обрабатываются именно так хотя бы комментарием, что это
# if not splitter
или подобным, чтобы легче было читать.
There was a problem hiding this comment.
Да нужно было пояснить как работает re.split в данном контексте.
| else: | ||
| # For others send original answer | ||
| shutil.copyfileobj(response, self.wfile) | ||
| con.close() |
There was a problem hiding this comment.
Совершенно забыл об обработке исключений. В случае исключения при запросе может иметь смысл отправлять повторный запрос(если GET, если POST - пусть сами разбираются) и отправлять осмысленные ошибки клиенту
There was a problem hiding this comment.
Предполагаю что проблемы будут с текстом ссылок вида domain.com/abcdef/index - вставит tm после abcdef
|
|
||
| elem_b = lxml.html.fromstring('<span class="sitestr">github.com/spencertipping</span>') | ||
| __add_tm_to_element__(elem_a) | ||
| self.assertEqual(b'<span class="sitestr">github.com/spencertipping</span>', lxml.html.tostring(elem_b)) |
There was a problem hiding this comment.
Косяк. Модифицируем elem_a, а проверяем elem_b
| __regexp_url__ = re.compile(r'(?i)\b((?:[a-z][\w-]+:(?:\/{1,3}|[a-z0-9%])|[a-z0-9.\-]+[.][a-z]{2,4}\/)' | ||
| r'(?:[^\s\(\)<>]+|\((?:[^\s\(\)<>]+|(?:\([^\s\(\)<>]+\)))*\))+' | ||
| r'(?:\((?:[^\s\(\)<>]+|(?:\([^\s\(\)<>]+\)))*\)|[^\s`!\(\)\[\]{};:\'".,<>?«»“”‘’]))') | ||
|
|
||
| __regexp_domain__ = re.compile(r'([\w-]+(?:\.[\w-]+)+)') | ||
| __regexp_dash_underline__ = re.compile(r'(-_+|_+-)') | ||
| __regexp_6letter_word__ = re.compile(r'(?i)(?<![\w-])([a-z][\w-]{4}[a-z0-9])(?![\w-])') |
There was a problem hiding this comment.
What's the point of naming variables/methods as in the case of class dunder methods?
Constants, for example, should be named in a caps lock case like REGEXP_URL
There was a problem hiding this comment.
In general, the solution is unusual and quite original, but I think the part with regular expressions (and working with html) is a bit overcomplicated, you could it in a much simpler way. Written tests in the project are a really good thing about it.
P.S. what about requirements.txt and README.md?
|
No description provided.