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

refactor component #5

Merged
merged 6 commits into from
Jan 25, 2022
Merged

refactor component #5

merged 6 commits into from
Jan 25, 2022

Conversation

AABur
Copy link
Contributor

@AABur AABur commented May 13, 2021

FIX #1

В геттерах и чекерах заменено обращение к значениям словаря через метод .get на обращение по ключу.
В get_children сохранено обращение через .get с установкой значения по умлочанию []

FIX #3

Исправлено неправильное test_get_meta на правильное test_get_children

FIX #4

Ошибки все понятные и устранить их не сложно. Но в данном случае не стоит. Для студентов будет сложнее понять код.
Предлагаю заигнорить в setup.cfg следующие:

  • B006 Do not use mutable data structures for argument defaults. They are created during function definition time. All calls to the function reuse this one instance of that data structure, persisting changes between them.
  • C812 missing trailing comma
  • D103 Missing docstring in public function
  • D300 Use “”"triple double quotes”“”
  • Q002 Remove bad quotes from docstring
  • WPS110 Found wrong variable name: result
  • WPS125 Found builtin shadowing: dir
  • WPS202 Found too many module members: 8 > 7
  • WPS226 Found string constant over-use: type > 3
  • WPS404 Found complex default value
  • WPS520 Found compare with falsy constant

@AABur AABur changed the title test: fix wrong name for test_get_children fix issues 1, 2, 3 May 13, 2021
@AABur AABur changed the title fix issues 1, 2, 3 fix #1 #2 #3 May 13, 2021
@AABur AABur changed the title fix #1 #2 #3 fix issues #1 #3 #4 May 13, 2021
'''Return children of node.'''
return directory.get('children')
"""Return children of node."""
return directory.get('children', [])
Copy link

Choose a reason for hiding this comment

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

Хорошо бы добавить тест, проверяющий это поведение, но не критично.

'name': name,
'meta': meta,
'type': 'file',
}


def mkdir(name, children=[], meta={}):
Copy link

Choose a reason for hiding this comment

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

Думаю, что лучше не игнорировать ошибку про мутабельные значения по-умолчанию. Иначе студенты могут смотреть на такое решение и думать, что это ок, и даже не будут знать про нюансы работы.
Если думаете, что будет непонятно, то вполне ок в комментариях пояснить или дать ссылку на какой-нибудь гайд, объясняющий эту особенность.

setup.cfg Outdated
@@ -7,6 +7,15 @@ ignore =
WPS412 # allow __init__.py with logic
DAR101 # allow missing parameters in Dockstring
DAR201 # allow missing Returns in Docstring
# ignored to make it easier for beginners to understand the code
Copy link

@vvkh vvkh May 15, 2021

Choose a reason for hiding this comment

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

Точно ли фиксы усложнят код здесь и стоит ли их глушить прямо для всего проекта?
Про WPS226 (string constant over-use) логичнее заигнорить для тестов только, т.к. именно там у нас много констант строковых и это в целом ок.
По остальным правилам я бы все-таки подумал еще, и к каждому в комментах хорошо бы написать, что это за правило и почему мы его выключаем.

@zhabinka zhabinka changed the title fix issues #1 #3 #4 refactor component (fix issues #1 #3 #4) Jan 25, 2022
@zhabinka zhabinka changed the title refactor component (fix issues #1 #3 #4) refactor component Jan 25, 2022
@zhabinka zhabinka merged commit f6560bd into hexlet-components:main Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants