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.

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

подпись #1873

Merged
merged 10 commits into from
Sep 20, 2019
Merged

подпись #1873

merged 10 commits into from
Sep 20, 2019

Conversation

gf-mse
Copy link
Contributor

@gf-mse gf-mse commented Sep 20, 2019

No description provided.

@fersel fersel added the enhancement New feature or request label Sep 20, 2019
@fersel
Copy link
Member

fersel commented Sep 20, 2019

Спасибо! Можешь, пожалуйста, создать отдельный PR для уточнения README.md (68fcfab)? Все остальное вмержим в рамках этого.

@gf-mse
Copy link
Contributor Author

gf-mse commented Sep 20, 2019

А резолвить не поможет (см. последний коммит) ?

update_readme.py Outdated
return sorted(signed, key=lambda pair: hashlib.sha256(repr(pair).encode('utf-8')).hexdigest())
signed_set.add((m.group(1).strip(), m.group(2).strip()))

signed_list = sorted(list( signed_set ), key=lambda pair: hashlib.sha256(repr(pair).encode('utf-8')).hexdigest())
Copy link
Contributor

Choose a reason for hiding this comment

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

нет нужды оборачивать signed_set в list, sorted принимает любой iterable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

да, меня уже просветили в #1890, спасибо ( исправлено )

update_readme.py Outdated
@@ -1,4 +1,5 @@
#!/usr/bin/env python3
# -*- coding: utf-8 -*-
Copy link
Contributor

Choose a reason for hiding this comment

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

если пайтон 3й, то данная строка необязательна

Copy link
Contributor Author

Choose a reason for hiding this comment

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

спасибо, сейчас учту

update_readme.py Outdated
\| # right-sep
"""

## pattern = re.compile( '%s | %s' % ( sig_pattern_1, sig_pattern_2 ), re.X )
Copy link
Contributor

Choose a reason for hiding this comment

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

закомменченный код на мой взгляд лучше удалять, ибо создаёт ненужный шум

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ок, согласен. ( Мне более привычно использовать | в регекспах вместе с именованными группами, так что это был некий реверанс в эту сторону. Но здесь это слишком громоздко, безусловно. )

@fersel
Copy link
Member

fersel commented Sep 20, 2019

А резолвить не поможет (см. последний коммит) ?

Дело не в резолвинге; мы с осторожностью относимся к изменениям в самом readme, я считаю, есть смысл обсудить это отдельно. Всё остальное из этого PR, с другой стороны, я готов вмержить в любой момент.

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

Так что в текущем виде я против внесения этой правки, однако если мы выделим инструкцию в отдельный файл (смотри обсуждение в #1965), мы учтём это в нём.

@gf-mse
Copy link
Contributor Author

gf-mse commented Sep 20, 2019

А резолвить не поможет (см. последний коммит) ?

Дело не в резолвинге; мы с осторожностью относимся к изменениям в самом readme, я считаю, есть смысл обсудить это отдельно.

Ок, принято.

Всё остальное из этого PR, с другой стороны, я готов вмержить в любой момент.

мне бы пригодились советы / ссылка на то, как легко и просто разбить один PR на несколько.
( впрочем, я могу поступить грязно и просто откатить этот файл. )

ps. Основная идея -- я собираюсь показывать этот проект куче народа (в основном -- англоязычной), и мне хотелось перед этим причесать код. "А то перед иностранцами неудобно" (ц) 😎

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

Нет проблем. Я вообще считаю, что вопросы эстетики -- целиком на откупе и совести создателей проекта ( любые OSS проекты имею в виду ).

Так что в текущем виде я против внесения этой правки, однако если мы выделим инструкцию в отдельный файл (смотри обсуждение в #1965), мы учтём это в нём.

Да я не настаиваю совершенно. В конце концов решать тому, кто этот скрипт запускает )

@fersel
Copy link
Member

fersel commented Sep 20, 2019

мне бы пригодились советы / ссылка на то, как легко и просто разбить один PR на несколько. ( впрочем, я могу поступить грязно и просто откатить этот файл. )

Я предлагал\имел в виду именно простой способ :)
Вообще создать вторую ветку на основе первой, первую откатить на нужное число коммитов, форспушнуть, пушнуть вторую.

@fersel fersel merged commit 5aaabb6 into developers-against-repressions:master Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants