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

Alina Layeuskaya #47

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

alinareal
Copy link

No description provided.

@alinareal alinareal changed the title [WIP] Alina Layeuskaya Alina Layeuskaya Jun 5, 2019

def main():
parser = create_parser()
namespace = parser.parse_args(argv[1:])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Зачем использовать sys.argv? ArgumentParser сам знает об аргументах командной строки.

Copy link
Author

Choose a reason for hiding this comment

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

Согласна. args = parser.parse_args()

import re
import builtins

from config import *
Copy link
Collaborator

Choose a reason for hiding this comment

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

Импорт через звездочку -- это зло

Copy link
Author

@alinareal alinareal Jun 10, 2019

Choose a reason for hiding this comment

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

Согласна. from config import (LETTERS, DIGITS, ....)

# it's more convenient to do it here in order to simplify parsing to tokens
if '..' in formula_string:
print('ERROR: Number can not contain more than one delimiter "." !')
was_error = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Вместо флагов лучше использовать исключения

Copy link
Author

Choose a reason for hiding this comment

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

Согласна про исключения и флаги. Не совсем понятно, какие исключения будут возбуждаться здесь, когда идут проверки на разделитель, неправильные операторы и т. д.

@AlexeiBuzuma
Copy link
Collaborator

  1. Сообщения коммитов в гите должны нести какой-то смысл изменения, которое было сделано в этом коммите. Сообщение Fix error handling 7 не несет особого смысла.
  2. В репозитории не должны лежить файлы, которые связаны сo средой разработки. Папка .idea никогда не должна попадать в репозиторий.
  3. Функция parse реализована как генератор с 20 конструкциями yield. Совершенно не уместно для этой задачи использовать такой сложный генератор.
  4. Для обработки ошибок используются флаги. В пайтоне есть замечательный механизм исключений, который был создан для упрощения работы с ошибками.
  5. x, y -- все это магические переменные, которые не несут никакого смысла. Название переменной должно четко описывать ее задачу.
  6. название тест кейса должно описывать то, что проверяется в тесте. Название test14 не несет никакой смысловой нагрузки.

@alinareal
Copy link
Author

  1. Да сообщения стоило бы поправить. В будущем учту.
  2. Какой-то косяк с гит игнор у меня вышел.
  3. Согласна по поводу parse и yield. Планировала зарефакторить в конце, но не успела.
  4. x, y - да согласна, перечитала правила наименования переменных, в будущем учту.
  5. Согласна по наименованию тестов, т.к. тесты очень однородные решила упростить себе задачу в их наименовании, в будущем учту.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants