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

466 criteria pack for md test #471

Merged
merged 25 commits into from
Mar 16, 2024
Merged

Conversation

MarinaProsche
Copy link
Collaborator

В продолжение (копия) #465
Ветка для проверки работоспособности нового пакета

@MarinaProsche MarinaProsche requested a review from zmm October 30, 2023 08:46
@github-actions github-actions bot added the has conflicts if new merge has conflicts label Nov 17, 2023
@github-actions github-actions bot removed the has conflicts if new merge has conflicts label Nov 17, 2023
@HadronCollider
Copy link
Collaborator

Можно, пожалуйста, пример набора критериев для проверки? (в формате для https://slides-checker.moevm.info/criterion_pack)

@MarinaProsche
Copy link
Collaborator Author

@HadronCollider Дмитрий, уточните, пожалуйста, в таком формате:

[
[
"simple_check"
],
[
"banned_words_in_literature"
],
[
"page_counter"
],
[
"short_sections_check"
],
[
"banned_words_check"
],
[
"right_words_check"
],
[
"banned_words_in_literature"
],
[
"literature_references"
],
[
"image_references"
],
[
"table_references"
],
[
"first_pages_check"
],
[
"main_character_check"
],
[
"needed_headers_check"
],
[
"report_section_component"
],
[
"spelling_check"
]
]

@github-actions github-actions bot added the has conflicts if new merge has conflicts label Mar 6, 2024
Copy link
Collaborator

@HadronCollider HadronCollider left a comment

Choose a reason for hiding this comment

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

Из того, что не отмечено в комментариях к коду

  • если нет таблиц / рисунков / списка литературы - занулять результаты критерия точно не нужно (как минимум, в работах с MD их может не быть, а мы ставим студентам 0), аналогично с ВКР -- возможно, это недосмотр с административной стороны прошлого года, но все-таки кажется, что в случае отсутствия критерий тривиально пройден
  • стоит уменьшить (или контролировать) "размеры" выводимой структуры в критерии проверки наличия разделов - для md там, по-видимому, вставляются "сырые" названия в html-формате (с h1, h2, etc) - в итоге браузер текст интерпретирует как html и там большушие заголовки - такое нам не надо (но надо отслеживать какой заголовок какого уровня - возможно, с помощью отступов)

@@ -83,6 +85,6 @@ def start_of_literature_chapter(self, ):
start_index = 0
for i in range(len(self.file.paragraphs)):
text_string = self.file.paragraphs[i].to_string().lower().split('\n')[1]
if re.fullmatch(self.name_pattern, text_string):
if re.fullmatch(f'{self.name_pattern}|{self.md_name_pattern}', text_string):
Copy link
Collaborator

Choose a reason for hiding this comment

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

А что будет, если загрузить не md-файл, простую ВКР в docx? это ведь общий критерий

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Удалила ненужный паттерн для md

@@ -12,6 +12,7 @@ def __init__(self, file_info, min_ref=1, max_ref=1000):
self.headers = []
self.literature_header = []
self.name_pattern = r'список[ \t]*(использованных|использованной|)[ \t]*(источников|литературы)'
self.md_name_pattern = r"<h2>(Список использованных источников|Список использованной литературы)<\/h2>"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Почему тут другой паттерн? https://github.com/moevm/mse_auto_checking_slides_vaganov/pull/471/files#diff-4a9d734369983549e0972bd59f7620e6ec4e30bed37af49ced2fcdc06ab558fbR16

Возможно, его (их / другие regexp) стоит вынести в класс файла (или uploader'а), чтобы использовать в нужных местах проверок и не теряться в их обилии

if isinstance(self.file.paragraphs[i], str):
detected_references = re.findall(r'\[[\d \-,]+\]', self.file.paragraphs[i])
else:
detected_references = re.findall(r'\[[\d \-,]+\]', self.file.paragraphs[i].to_string().split('\n')[1])
Copy link
Collaborator

Choose a reason for hiding this comment

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

А в app/main/checks/report_checks/image_references.py используется self.file.paragraphs[i].paragraph_text - в чем разница?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Исправила на "paragraph_text", работает корректно

@@ -86,16 +90,22 @@ def search_references(self, start_par):
for k in range(int(start), int(end) + 1):
array_of_references.add(k)
elif one_part != '':
array_of_references.add(int(one_part))
array_of_references.add(int(one_part))
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
Collaborator Author

Choose a reason for hiding this comment

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

Исправила

if re.fullmatch(self.name_pattern, text_string):
start_index = i
break
if isinstance(self.file.paragraphs[i], str):
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • В app/main/checks/report_checks/banned_words_in_literature.py логики для name_pattern нет, а тут есть

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Исправила, работает корректно

def get_headers(self):
header_regex = "<h1>(.*?)<\/h1>"
self.headers = re.findall(header_regex, self.html_text)
def page_counter(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Предлагаю совсем не считать (как минимум сейчас) количество страниц в md - это, кажется, несколько бессмысленным (+ замедляет проверку из-за построчного анализа страниц pdf версии)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Сейчас этот подсчет нужен для того, чтобы открывать в качестве ссылки страницу со списком литературы в проверке источников (обсуждали в переписке). Удалить?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Давайте пометим на будущее комментарием в коде, что/зачем происходит - чтобы не забыть

self.pdf_file = PdfDocumentManager(self.path_to_md_file, md2pdf(self.pdf_filepath, md_file_path=self.path_to_md_file))

def make_paragraphs(self, html_text):
html_text = html_text.replace("<li>", "").replace("</li>", "").replace("</ol>", "").replace("<ol>", "")
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
Collaborator Author

Choose a reason for hiding this comment

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

Пока выглядит, как будто мы сделали хорошо, потому что в некоторых документах (не всех, что тоже придает хаоса) при парсинге тэги сохраняются в отдельные строки, а это как раз приводит к некорректным проверкам

Comment on lines 8 to 10
if file.mimetype != 'text/markdown':
return "mime_type_does_not_match_extension"
Copy link
Collaborator

Choose a reason for hiding this comment

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

С простым текстовым файлом (пусть и содержащим текст с markdown форматированием) у меня возникли проблемы - тип там "text/plain" и проверка не проходит

return "mime_type_does_not_match_extension"

return "ok"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Чтобы было проще обрабатывать результат этой функции пусть она возвращает None (или пустую строку), если тип допустимый - иначе проверки вида != / == "ok" сильно усложняют код и читаемость

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Исправила

if isinstance(self.file.paragraphs[i], str):
detected_references = re.findall(r'таблиц[аеыу][\d .]+', self.file.paragraphs[i])
else:
detected_references = re.findall(r'таблиц[аеыу][\d .]+', self.file.paragraphs[i].paragraph_text)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Аналогично про

А в `app/main/checks/report_checks/image_references.py` используется
`self.file.paragraphs[i].paragraph_text` - в чем разница?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Оставила "paragraph_text"

@github-actions github-actions bot removed the has conflicts if new merge has conflicts label Mar 13, 2024
@HadronCollider HadronCollider merged commit 727e18c into master Mar 16, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants