From 90702c34f0fbc789f76d3bd76edae921301a2541 Mon Sep 17 00:00:00 2001 From: Jonas Dittrich <58814480+Kakadus@users.noreply.github.com> Date: Mon, 8 Apr 2024 21:35:11 +0200 Subject: [PATCH] Introduce ruff to ci (#2051) * Initial attempt at ruff * prohibit FIXME, TODO and XXX * add zip strict parameter * add ignores in settings * add __all__ * add stacklevel to warning, when top level code is executed twice * add DJ checks * ruff formatter * enable "free" tests * enable and (auto) fix set|list|map literal * enable and (auto) fix implicit string concat * enable and (auto) fix unnecessary start in range * enable and (auto) fix exception parentheses * enable and (auto) fix unnecessary assignment * enable COMpatible COM * enable and fix unspecific ignore * enable and fix string join * enable and fix list comprehensions * attr defined mypy ignore * enable PL, but keep invalid-name * enable PERF203 and BLE instead of broad-exception-caught * Use correct weekday range * run ruff first and then pylint. Also do this with manage.py * do not print in commands, use stdout instead! * lint enrollment_preprocessor --------- Co-authored-by: Richard Ebeling --- .github/workflows/tests.yml | 5 +- evap/contributor/tests/test_forms.py | 4 +- evap/evaluation/auth.py | 8 +-- .../management/commands/anonymize.py | 14 ++--- evap/evaluation/management/commands/lint.py | 3 + .../management/commands/send_reminders.py | 14 ++--- evap/evaluation/migrations/0001_initial.py | 4 +- .../migrations/0017_delete_old_degrees.py | 2 +- .../0026_make_result_counters_unique.py | 4 +- .../0031_add_rating_answer_counter.py | 2 +- .../0033_remove_likert_and_grade_answer.py | 4 +- .../migrations/0100_clear_evaluation_names.py | 2 +- evap/evaluation/models.py | 59 +++++++++---------- evap/evaluation/models_logging.py | 26 ++++---- evap/evaluation/tests/test_auth.py | 2 +- evap/evaluation/tests/test_commands.py | 15 ++--- evap/evaluation/tools.py | 8 +-- evap/evaluation/views.py | 3 +- ...0005_make_description_unique_per_course.py | 2 +- .../0008_add_gradedocument_description_en.py | 2 +- ...gradedocument_description_en_add_unique.py | 2 +- evap/grades/tests.py | 3 +- evap/locale/de/formats.py | 2 +- evap/results/exporters.py | 11 ++-- .../templatetags/results_templatetags.py | 3 +- evap/results/tests/test_exporters.py | 6 +- evap/results/tools.py | 9 +-- evap/results/views.py | 4 +- evap/rewards/tools.py | 8 +-- evap/rewards/views.py | 21 ++++--- evap/settings.py | 6 +- evap/staff/fixtures/__init__.py | 0 evap/staff/fixtures/excel_files_test_data.py | 2 +- evap/staff/forms.py | 10 ++-- evap/staff/importers/__init__.py | 9 +++ evap/staff/importers/base.py | 8 +-- evap/staff/importers/enrollment.py | 8 +-- evap/staff/staff_mode.py | 3 +- evap/staff/tests/test_importers.py | 28 ++++----- evap/staff/tests/test_tools.py | 4 +- evap/staff/tests/test_views.py | 14 ++--- evap/staff/tools.py | 15 +++-- evap/staff/views.py | 12 ++-- evap/student/forms.py | 2 +- pyproject.toml | 35 ++++++++++- requirements-dev.txt | 1 + tools/enrollment_preprocessor.py | 3 +- 47 files changed, 224 insertions(+), 188 deletions(-) create mode 100644 evap/staff/fixtures/__init__.py diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 04d9d663fc..685fa03c9c 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -88,7 +88,10 @@ jobs: - name: Setup python uses: ./.github/setup_python - - name: Run linter + - name: Run ruff + run: ruff check . + + - name: Run pylint run: pylint evap tools diff --git a/evap/contributor/tests/test_forms.py b/evap/contributor/tests/test_forms.py index 900d13ef40..8dc7a58591 100644 --- a/evap/contributor/tests/test_forms.py +++ b/evap/contributor/tests/test_forms.py @@ -67,7 +67,7 @@ def test_managers_only(self): ) formset = InlineContributionFormset(instance=evaluation, form_kwargs={"evaluation": evaluation}) - expected = set([questionnaire]) + expected = {questionnaire} self.assertEqual(expected, set(formset.forms[0].fields["questionnaires"].queryset)) self.assertEqual(expected, set(formset.forms[1].fields["questionnaires"].queryset)) @@ -79,7 +79,7 @@ def test_managers_only(self): ) formset = InlineContributionFormset(instance=evaluation, form_kwargs={"evaluation": evaluation}) - expected = set([questionnaire, questionnaire_managers_only]) + expected = {questionnaire, questionnaire_managers_only} self.assertEqual(expected, set(formset.forms[0].fields["questionnaires"].queryset)) self.assertEqual(expected, set(formset.forms[1].fields["questionnaires"].queryset)) diff --git a/evap/evaluation/auth.py b/evap/evaluation/auth.py index b71a8dea46..b70efde1b3 100644 --- a/evap/evaluation/auth.py +++ b/evap/evaluation/auth.py @@ -1,6 +1,6 @@ import inspect +from collections.abc import Callable from functools import wraps -from typing import Callable from django.contrib.auth.backends import ModelBackend from django.core.exceptions import PermissionDenied @@ -28,8 +28,7 @@ def authenticate(self, request, key): # pylint: disable=arguments-differ return None try: - user = UserProfile.objects.get(login_key=key) - return user + return UserProfile.objects.get(login_key=key) except UserProfile.DoesNotExist: return None @@ -144,12 +143,11 @@ def filter_users_by_claims(self, claims): return [] def create_user(self, claims): - user = self.UserModel.objects.create( + return self.UserModel.objects.create( email=claims.get("email"), first_name_given=claims.get("given_name", ""), last_name=claims.get("family_name", ""), ) - return user def update_user(self, user, claims): if not user.first_name_given: diff --git a/evap/evaluation/management/commands/anonymize.py b/evap/evaluation/management/commands/anonymize.py index 272bc9f2e2..fc248989eb 100644 --- a/evap/evaluation/management/commands/anonymize.py +++ b/evap/evaluation/management/commands/anonymize.py @@ -94,8 +94,8 @@ def anonymize_users(self, first_names, last_names): if len(first_names) * len(last_names) < len(user_profiles) * 1.5: self.stdout.write( "Warning: There are few example names compared to all that real data to be anonymized. " - + "Consider adding more data to the first_names.txt and last_names.txt files in the anonymize_data " - + "folder." + "Consider adding more data to the first_names.txt and last_names.txt files in the anonymize_data " + "folder." ) while len(fake_usernames) < len(user_profiles): @@ -111,7 +111,7 @@ def anonymize_users(self, first_names, last_names): # Actually replace all the real user data self.stdout.write("Replacing email addresses and login keys with fake ones...") - for user, name in zip(user_profiles, fake_usernames): + for user, name in zip(user_profiles, fake_usernames, strict=True): if user.email and user.email.split("@")[0] in Command.ignore_email_usernames: continue user.first_name_given = name[0] @@ -161,7 +161,7 @@ def anonymize_courses(self): # Shuffle public courses' names in order to decouple them from the results. # Also, assign public courses' names to private ones as their names may be confidential. self.stdout.write("Shuffling course names...") - public_names = list(set(map(lambda c: (c.name_de, c.name_en), public_courses))) + public_names = list({(c.name_de, c.name_en) for c in public_courses}) random.shuffle(public_names) for i, course in enumerate(courses): @@ -188,7 +188,7 @@ def anonymize_evaluations(self): self.stdout.write("Shuffling evaluation names...") named_evaluations = (evaluation for evaluation in evaluations if evaluation.name_de and evaluation.name_en) - names = list(set(map(lambda c: (c.name_de, c.name_en), named_evaluations))) + names = list({(c.name_de, c.name_en) for c in named_evaluations}) random.shuffle(names) for i, evaluation in enumerate(evaluations): @@ -244,7 +244,7 @@ def anonymize_answers(self, lorem_ipsum): for question, counters in counters_per_question.items(): original_sum = sum(counter.count for counter in counters) - missing_values = set(CHOICES[question.type].values).difference(set(c.answer for c in counters)) + missing_values = set(CHOICES[question.type].values).difference({c.answer for c in counters}) missing_values.discard(NO_ANSWER) # don't add NO_ANSWER counter if it didn't exist before for value in missing_values: counters.append( @@ -259,7 +259,7 @@ def anonymize_answers(self, lorem_ipsum): index = random.randint(0, len(generated_counts) - 1) # nosec generated_counts[index] += to_add - for counter, generated_count in zip(counters, generated_counts): + for counter, generated_count in zip(counters, generated_counts, strict=True): assert generated_count >= 0 counter.count = generated_count diff --git a/evap/evaluation/management/commands/lint.py b/evap/evaluation/management/commands/lint.py index 767637d61f..f5045417f0 100644 --- a/evap/evaluation/management/commands/lint.py +++ b/evap/evaluation/management/commands/lint.py @@ -9,4 +9,7 @@ class Command(BaseCommand): requires_migrations_checks = False def handle(self, *args, **options): + self.stdout.write("Executing ruff .") + subprocess.run(["ruff", "check", "."], check=False) # nosec + self.stdout.write("Executing pylint evap") subprocess.run(["pylint", "evap", "tools"], check=False) # nosec diff --git a/evap/evaluation/management/commands/send_reminders.py b/evap/evaluation/management/commands/send_reminders.py index deb67d428b..73e9d537c9 100644 --- a/evap/evaluation/management/commands/send_reminders.py +++ b/evap/evaluation/management/commands/send_reminders.py @@ -25,10 +25,7 @@ def get_sorted_evaluation_url_tuples_with_urgent_review() -> list[tuple[Evaluati for evaluation in Evaluation.objects.filter(state=Evaluation.State.EVALUATED) if evaluation.textanswer_review_state == Evaluation.TextAnswerReviewState.REVIEW_URGENT ] - evaluation_url_tuples = sorted( - evaluation_url_tuples, key=lambda evaluation_url_tuple: evaluation_url_tuple[0].full_name - ) - return evaluation_url_tuples + return sorted(evaluation_url_tuples, key=lambda evaluation_url_tuple: evaluation_url_tuple[0].full_name) @log_exceptions @@ -43,11 +40,10 @@ def handle(self, *args, **options): @staticmethod def send_student_reminders(): - check_dates = [] - - # Collect end-dates of evaluations whose participants need to be reminded today. - for number_of_days in settings.REMIND_X_DAYS_AHEAD_OF_END_DATE: - check_dates.append(datetime.date.today() + datetime.timedelta(days=number_of_days)) + check_dates = [ + datetime.date.today() + datetime.timedelta(days=number_of_days) + for number_of_days in settings.REMIND_X_DAYS_AHEAD_OF_END_DATE + ] recipients = set() for evaluation in Evaluation.objects.filter( diff --git a/evap/evaluation/migrations/0001_initial.py b/evap/evaluation/migrations/0001_initial.py index 9c7ff500a6..f439af90b0 100644 --- a/evap/evaluation/migrations/0001_initial.py +++ b/evap/evaluation/migrations/0001_initial.py @@ -257,7 +257,7 @@ class Migration(migrations.Migration): ), migrations.AlterUniqueTogether( name='course', - unique_together=set([('semester', 'degree', 'name_de'), ('semester', 'degree', 'name_en')]), + unique_together={('semester', 'degree', 'name_de'), ('semester', 'degree', 'name_en')}, ), migrations.AddField( model_name='contribution', @@ -273,6 +273,6 @@ class Migration(migrations.Migration): ), migrations.AlterUniqueTogether( name='contribution', - unique_together=set([('course', 'contributor')]), + unique_together={('course', 'contributor')}, ), ] diff --git a/evap/evaluation/migrations/0017_delete_old_degrees.py b/evap/evaluation/migrations/0017_delete_old_degrees.py index 53633687a8..756cb52771 100644 --- a/evap/evaluation/migrations/0017_delete_old_degrees.py +++ b/evap/evaluation/migrations/0017_delete_old_degrees.py @@ -14,7 +14,7 @@ class Migration(migrations.Migration): ), migrations.AlterUniqueTogether( name='course', - unique_together=set([('semester', 'name_en'), ('semester', 'name_de')]), + unique_together={('semester', 'name_en'), ('semester', 'name_de')}, ), migrations.RemoveField( model_name='course', diff --git a/evap/evaluation/migrations/0026_make_result_counters_unique.py b/evap/evaluation/migrations/0026_make_result_counters_unique.py index 2d1c894167..0b2db2a540 100644 --- a/evap/evaluation/migrations/0026_make_result_counters_unique.py +++ b/evap/evaluation/migrations/0026_make_result_counters_unique.py @@ -10,10 +10,10 @@ class Migration(migrations.Migration): operations = [ migrations.AlterUniqueTogether( name='gradeanswercounter', - unique_together=set([('question', 'contribution', 'answer')]), + unique_together={('question', 'contribution', 'answer')}, ), migrations.AlterUniqueTogether( name='likertanswercounter', - unique_together=set([('question', 'contribution', 'answer')]), + unique_together={('question', 'contribution', 'answer')}, ), ] diff --git a/evap/evaluation/migrations/0031_add_rating_answer_counter.py b/evap/evaluation/migrations/0031_add_rating_answer_counter.py index 7f3559a052..1c507d0f8a 100644 --- a/evap/evaluation/migrations/0031_add_rating_answer_counter.py +++ b/evap/evaluation/migrations/0031_add_rating_answer_counter.py @@ -25,6 +25,6 @@ class Migration(migrations.Migration): ), migrations.AlterUniqueTogether( name='ratinganswercounter', - unique_together=set([('question', 'contribution', 'answer')]), + unique_together={('question', 'contribution', 'answer')}, ), ] diff --git a/evap/evaluation/migrations/0033_remove_likert_and_grade_answer.py b/evap/evaluation/migrations/0033_remove_likert_and_grade_answer.py index 53e704e674..c355e740a8 100644 --- a/evap/evaluation/migrations/0033_remove_likert_and_grade_answer.py +++ b/evap/evaluation/migrations/0033_remove_likert_and_grade_answer.py @@ -10,7 +10,7 @@ class Migration(migrations.Migration): operations = [ migrations.AlterUniqueTogether( name='gradeanswercounter', - unique_together=set([]), + unique_together=set(), ), migrations.RemoveField( model_name='gradeanswercounter', @@ -22,7 +22,7 @@ class Migration(migrations.Migration): ), migrations.AlterUniqueTogether( name='likertanswercounter', - unique_together=set([]), + unique_together=set(), ), migrations.RemoveField( model_name='likertanswercounter', diff --git a/evap/evaluation/migrations/0100_clear_evaluation_names.py b/evap/evaluation/migrations/0100_clear_evaluation_names.py index 11ef39ae82..50b96f0074 100644 --- a/evap/evaluation/migrations/0100_clear_evaluation_names.py +++ b/evap/evaluation/migrations/0100_clear_evaluation_names.py @@ -22,7 +22,7 @@ def name_evaluations(apps, _schema_editor): evaluation.name_en = course.name_en evaluation.save() else: - for i in range(0, course.evaluations.count()): + for i in range(course.evaluations.count()): evaluation = Evaluation.objects.get(pk=course.evaluations.all()[i].pk) evaluation.name_de = f"{course.name_de} ({i})" evaluation.name_en = f"{course.name_en} ({i})" diff --git a/evap/evaluation/models.py b/evap/evaluation/models.py index 58675e5242..b178db1d94 100644 --- a/evap/evaluation/models.py +++ b/evap/evaluation/models.py @@ -106,7 +106,7 @@ def results_can_be_archived(self): @transaction.atomic def archive(self): if not self.participations_can_be_archived: - raise NotArchivableError() + raise NotArchivableError for evaluation in self.evaluations.all(): evaluation._archive() self.participations_are_archived = True @@ -119,14 +119,14 @@ def delete_grade_documents(self): from evap.grades.models import GradeDocument if not self.grade_documents_can_be_deleted: - raise NotArchivableError() + raise NotArchivableError GradeDocument.objects.filter(course__semester=self).delete() self.grade_documents_are_deleted = True self.save() def archive_results(self): if not self.results_can_be_archived: - raise NotArchivableError() + raise NotArchivableError self.results_are_archived = True self.save() @@ -194,10 +194,6 @@ class Visibility(models.IntegerChoices): objects = QuestionnaireManager() - def clean(self): - if self.type == self.Type.CONTRIBUTOR and self.is_locked: - raise ValidationError({"is_locked": _("Contributor questionnaires cannot be locked.")}) - class Meta: ordering = ["type", "order", "pk"] verbose_name = _("questionnaire") @@ -206,6 +202,10 @@ class Meta: def __str__(self): return self.name + def clean(self): + if self.type == self.Type.CONTRIBUTOR and self.is_locked: + raise ValidationError({"is_locked": _("Contributor questionnaires cannot be locked.")}) + def __lt__(self, other): return (self.type, self.order, self.pk) < (other.type, other.order, other.pk) @@ -636,7 +636,7 @@ def num_participants(self): def _archive(self): """Should be called only via Semester.archive""" if not self.participations_can_be_archived: - raise NotArchivableError() + raise NotArchivableError if self._participant_count is not None: assert self._voter_count is not None assert ( @@ -956,7 +956,7 @@ def update_evaluations(cls): evaluation.publish() evaluation_results_evaluations.append(evaluation) evaluation.save() - except Exception: # pylint: disable=broad-except + except Exception: # noqa: PERF203 logger.exception( 'An error occured when updating the state of evaluation "%s" (id %d).', evaluation, evaluation.id ) @@ -1295,7 +1295,7 @@ class BipolarChoices(Choices): _("No answer"), ], is_inverted=False, - **BASE_UNIPOLAR_CHOICES, # type: ignore + **BASE_UNIPOLAR_CHOICES, # type: ignore[arg-type] ), QuestionType.NEGATIVE_LIKERT: Choices( names=[ @@ -1307,7 +1307,7 @@ class BipolarChoices(Choices): _("No answer"), ], is_inverted=True, - **BASE_UNIPOLAR_CHOICES, # type: ignore + **BASE_UNIPOLAR_CHOICES, # type: ignore[arg-type] ), QuestionType.GRADE: Choices( names=[ @@ -1319,7 +1319,7 @@ class BipolarChoices(Choices): _("No answer"), ], is_inverted=False, - **BASE_UNIPOLAR_CHOICES, # type: ignore + **BASE_UNIPOLAR_CHOICES, # type: ignore[arg-type] ), QuestionType.EASY_DIFFICULT: BipolarChoices( minus_name=_("Easy"), @@ -1334,7 +1334,7 @@ class BipolarChoices(Choices): _("Way too\ndifficult"), _("No answer"), ], - **BASE_BIPOLAR_CHOICES, # type: ignore + **BASE_BIPOLAR_CHOICES, # type: ignore[arg-type] ), QuestionType.FEW_MANY: BipolarChoices( minus_name=_("Few"), @@ -1349,7 +1349,7 @@ class BipolarChoices(Choices): _("Way too\nmany"), _("No answer"), ], - **BASE_BIPOLAR_CHOICES, # type: ignore + **BASE_BIPOLAR_CHOICES, # type: ignore[arg-type] ), QuestionType.LITTLE_MUCH: BipolarChoices( minus_name=_("Little"), @@ -1364,7 +1364,7 @@ class BipolarChoices(Choices): _("Way too\nmuch"), _("No answer"), ], - **BASE_BIPOLAR_CHOICES, # type: ignore + **BASE_BIPOLAR_CHOICES, # type: ignore[arg-type] ), QuestionType.SMALL_LARGE: BipolarChoices( minus_name=_("Small"), @@ -1379,7 +1379,7 @@ class BipolarChoices(Choices): _("Way too\nlarge"), _("No answer"), ], - **BASE_BIPOLAR_CHOICES, # type: ignore + **BASE_BIPOLAR_CHOICES, # type: ignore[arg-type] ), QuestionType.SLOW_FAST: BipolarChoices( minus_name=_("Slow"), @@ -1394,7 +1394,7 @@ class BipolarChoices(Choices): _("Way too\nfast"), _("No answer"), ], - **BASE_BIPOLAR_CHOICES, # type: ignore + **BASE_BIPOLAR_CHOICES, # type: ignore[arg-type] ), QuestionType.SHORT_LONG: BipolarChoices( minus_name=_("Short"), @@ -1409,7 +1409,7 @@ class BipolarChoices(Choices): _("Way too\nlong"), _("No answer"), ], - **BASE_BIPOLAR_CHOICES, # type: ignore + **BASE_BIPOLAR_CHOICES, # type: ignore[arg-type] ), QuestionType.POSITIVE_YES_NO: Choices( names=[ @@ -1418,7 +1418,7 @@ class BipolarChoices(Choices): _("No answer"), ], is_inverted=False, - **BASE_YES_NO_CHOICES, # type: ignore + **BASE_YES_NO_CHOICES, # type: ignore[arg-type] ), QuestionType.NEGATIVE_YES_NO: Choices( names=[ @@ -1427,7 +1427,7 @@ class BipolarChoices(Choices): _("No answer"), ], is_inverted=True, - **BASE_YES_NO_CHOICES, # type: ignore + **BASE_YES_NO_CHOICES, # type: ignore[arg-type] ), } @@ -1474,7 +1474,7 @@ class TextAnswer(Answer): answer = models.TextField(verbose_name=_("answer")) # If the text answer was changed during review, original_answer holds the original text. Otherwise, it's null. - original_answer = models.TextField(verbose_name=_("original answer"), blank=True, null=True) + original_answer = models.TextField(verbose_name=_("original answer"), blank=True, null=True) # noqa: DJ001 class ReviewDecision(models.TextChoices): """ @@ -1533,9 +1533,6 @@ def is_private(self): def is_reviewed(self): return self.review_decision != self.ReviewDecision.UNDECIDED - def save(self, *args, **kwargs): - super().save(*args, **kwargs) - class FaqSection(models.Model): """Section in the frequently asked questions""" @@ -1617,9 +1614,6 @@ class Infotext(models.Model): content_en = models.TextField(verbose_name=_("content (english)"), blank=True) content = translate(en="content_en", de="content_de") - def is_empty(self): - return not (self.title or self.content) - class Page(models.TextChoices): STUDENT_INDEX = ("student_index", "Student index page") CONTRIBUTOR_INDEX = ("contributor_index", "Contributor index page") @@ -1644,6 +1638,9 @@ class Meta: ), ) + def is_empty(self): + return not (self.title or self.content) + class UserProfileManager(BaseUserManager): def create_user(self, *, email, password=None, first_name=None, last_name=None): @@ -1909,7 +1906,7 @@ def ensure_valid_login_key(self): return while True: - key = secrets.choice(range(0, UserProfile.MAX_LOGIN_KEY)) + key = secrets.choice(range(UserProfile.MAX_LOGIN_KEY)) try: self.login_key = key self.reset_login_key_validity() @@ -2105,7 +2102,7 @@ def send_to_user(self, user, subject_params, body_params, use_cc, additional_cc_ logger.info('Sent email "%s" to %s (%s).', mail.subject, user.full_name, user.email) if send_separate_login_url: self.send_login_url_to_user(user) - except Exception: # pylint: disable=broad-except + except Exception: logger.exception( 'An exception occurred when sending the following email to user "%s":\n%s\n', user.full_name_with_additional_info, @@ -2125,7 +2122,7 @@ def construct_mail(self, to_email, cc_addresses, subject_params, body_params): if settings.SEND_ALL_EMAILS_TO_ADMINS_IN_BCC: bcc_addresses = [a[1] for a in settings.ADMINS] - mail = EmailMultiAlternatives( + return EmailMultiAlternatives( subject=subject, body=plain_content, to=[to_email], @@ -2135,8 +2132,6 @@ def construct_mail(self, to_email, cc_addresses, subject_params, body_params): alternatives=[(wrapped_content, "text/html")], ) - return mail - @classmethod def send_reminder_to_user(cls, user, first_due_in_days, due_evaluations): template = cls.objects.get(name=cls.STUDENT_REMINDER) diff --git a/evap/evaluation/models_logging.py b/evap/evaluation/models_logging.py index f9cd00332c..e54ebb0f47 100644 --- a/evap/evaluation/models_logging.py +++ b/evap/evaluation/models_logging.py @@ -128,6 +128,19 @@ class LoggedModel(models.Model): class Meta: abstract = True + def save(self, *args, **kw): + # Are we creating a new instance? + # https://docs.djangoproject.com/en/3.0/ref/models/instances/#customizing-model-loading + if self._state.adding: + # we need to attach a logentry to an existing object, so we save this newly created instance first + super().save(*args, **kw) + self.log_instance_create() + else: + # when saving an existing instance, we get changes by comparing to the version from the database + # therefore we save the instance after building the logentry + self.log_instance_change() + super().save(*args, **kw) + def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self._logentry = None @@ -239,19 +252,6 @@ def _update_log(self, changes, action_type: InstanceActionType, store_in_db=True if store_in_db: self._logentry.save() - def save(self, *args, **kw): - # Are we creating a new instance? - # https://docs.djangoproject.com/en/3.0/ref/models/instances/#customizing-model-loading - if self._state.adding: - # we need to attach a logentry to an existing object, so we save this newly created instance first - super().save(*args, **kw) - self.log_instance_create() - else: - # when saving an existing instance, we get changes by comparing to the version from the database - # therefore we save the instance after building the logentry - self.log_instance_change() - super().save(*args, **kw) - def delete(self, *args, **kw): self.log_instance_delete() self.related_logentries().delete() diff --git a/evap/evaluation/tests/test_auth.py b/evap/evaluation/tests/test_auth.py index 899b6e4753..a49d14b612 100644 --- a/evap/evaluation/tests/test_auth.py +++ b/evap/evaluation/tests/test_auth.py @@ -187,7 +187,7 @@ class TestAuthDecorators(WebTest): def setUpTestData(cls): @class_or_function_check_decorator def check_decorator(user: UserProfile) -> bool: - return getattr(user, "some_condition") # mocked later + return getattr(user, "some_condition") # noqa: B009 # mocked later @check_decorator def function_based_view(_request): diff --git a/evap/evaluation/tests/test_commands.py b/evap/evaluation/tests/test_commands.py index 85202a8577..b5ce1e6afa 100644 --- a/evap/evaluation/tests/test_commands.py +++ b/evap/evaluation/tests/test_commands.py @@ -4,7 +4,7 @@ from datetime import date, datetime, timedelta from io import StringIO from itertools import chain, cycle -from unittest.mock import call, patch +from unittest.mock import MagicMock, call, patch from django.conf import settings from django.core import mail, management @@ -336,7 +336,7 @@ def test_dont_remind_already_voted(self): self.assertEqual(mock.call_count, 0) self.assertEqual(len(mail.outbox), 0) - @override_settings(TEXTANSWER_REVIEW_REMINDER_WEEKDAYS=list(range(0, 8))) + @override_settings(TEXTANSWER_REVIEW_REMINDER_WEEKDAYS=list(range(7))) def test_send_text_answer_review_reminder(self): make_manager() evaluation = baker.make( @@ -366,11 +366,12 @@ def test_send_text_answer_review_reminder(self): class TestLintCommand(TestCase): - @staticmethod @patch("subprocess.run") - def test_pylint_called(mock_subprocess_run): - management.call_command("lint") - mock_subprocess_run.assert_called_once_with(["pylint", "evap", "tools"], check=False) + def test_pylint_called(self, mock_subprocess_run: MagicMock): + management.call_command("lint", stdout=StringIO()) + self.assertEqual(mock_subprocess_run.call_count, 2) + mock_subprocess_run.assert_any_call(["ruff", "check", "."], check=False) + mock_subprocess_run.assert_any_call(["pylint", "evap", "tools"], check=False) class TestFormatCommand(TestCase): @@ -409,7 +410,7 @@ def test_subcommands_called(self, mock_call_command, mock_subprocess_run): mock_call_command.assert_any_call("format") -@override_settings(TEXTANSWER_REVIEW_REMINDER_WEEKDAYS=range(0, 7)) +@override_settings(TEXTANSWER_REVIEW_REMINDER_WEEKDAYS=range(7)) class TestSendTextanswerRemindersCommand(TestCase): def test_send_reminder(self): make_manager() diff --git a/evap/evaluation/tools.py b/evap/evaluation/tools.py index 9012ccedce..47c48baeef 100644 --- a/evap/evaluation/tools.py +++ b/evap/evaluation/tools.py @@ -81,15 +81,15 @@ def discard_cached_related_objects(instance: M) -> M: hierarchy (e.g. for storing instances in a cache) """ # Extracted from django's refresh_from_db, which sadly doesn't offer this part alone (without hitting the DB). - for field in instance._meta.concrete_fields: # type: ignore + for field in instance._meta.concrete_fields: # type: ignore[attr-defined] if field.is_relation and field.is_cached(instance): field.delete_cached_value(instance) - for field in instance._meta.related_objects: # type: ignore + for field in instance._meta.related_objects: # type: ignore[attr-defined] if field.is_cached(instance): field.delete_cached_value(instance) - instance._prefetched_objects_cache = {} # type: ignore + instance._prefetched_objects_cache = {} # type: ignore[attr-defined] return instance @@ -244,7 +244,7 @@ def __init__(self, *args, **kwargs) -> None: super().__init__(*args, **kwargs) del self["content-type"] - @HttpResponse.content.setter # type: ignore + @HttpResponse.content.setter # type: ignore[attr-defined] def content(self, value): if value: raise AttributeError("You cannot set content to a 204 (No Content) response") diff --git a/evap/evaluation/views.py b/evap/evaluation/views.py index a21616c1c9..77d6cd48da 100644 --- a/evap/evaluation/views.py +++ b/evap/evaluation/views.py @@ -23,8 +23,7 @@ logger = logging.getLogger(__name__) -def redirect_user_to_start_page(user): - # pylint: disable=too-many-return-statements +def redirect_user_to_start_page(user): # noqa: PLR0911 active_semester = Semester.active_semester() if user.is_reviewer: diff --git a/evap/grades/migrations/0005_make_description_unique_per_course.py b/evap/grades/migrations/0005_make_description_unique_per_course.py index 06227dc0ff..b76d14c20d 100644 --- a/evap/grades/migrations/0005_make_description_unique_per_course.py +++ b/evap/grades/migrations/0005_make_description_unique_per_course.py @@ -10,6 +10,6 @@ class Migration(migrations.Migration): operations = [ migrations.AlterUniqueTogether( name='gradedocument', - unique_together=set([('course', 'description')]), + unique_together={('course', 'description')}, ), ] diff --git a/evap/grades/migrations/0008_add_gradedocument_description_en.py b/evap/grades/migrations/0008_add_gradedocument_description_en.py index 385e1a39da..301df59895 100644 --- a/evap/grades/migrations/0008_add_gradedocument_description_en.py +++ b/evap/grades/migrations/0008_add_gradedocument_description_en.py @@ -17,7 +17,7 @@ class Migration(migrations.Migration): ), migrations.AlterUniqueTogether( name='gradedocument', - unique_together=set([('course', 'description_de')]), + unique_together={('course', 'description_de')}, ), migrations.AlterField( model_name='gradedocument', diff --git a/evap/grades/migrations/0010_gradedocument_description_en_add_unique.py b/evap/grades/migrations/0010_gradedocument_description_en_add_unique.py index f7e084176b..8d99b54522 100644 --- a/evap/grades/migrations/0010_gradedocument_description_en_add_unique.py +++ b/evap/grades/migrations/0010_gradedocument_description_en_add_unique.py @@ -12,6 +12,6 @@ class Migration(migrations.Migration): operations = [ migrations.AlterUniqueTogether( name='gradedocument', - unique_together=set([('course', 'description_de'), ('course', 'description_en')]), + unique_together={('course', 'description_de'), ('course', 'description_en')}, ), ] diff --git a/evap/grades/tests.py b/evap/grades/tests.py index 1e02bfc279..1e945a95e7 100644 --- a/evap/grades/tests.py +++ b/evap/grades/tests.py @@ -61,14 +61,13 @@ def helper_upload_grades(self, course, final_grades): upload_files = [("file", "grades.txt", b"Some content")] final = "?final=true" if final_grades else "" - response = self.app.post( + return self.app.post( f"{reverse('grades:upload_grades', args=[course.id])}{final}", params={"description_en": "Grades", "description_de": "Grades"}, user=self.grade_publisher, content_type="multipart/form-data", upload_files=upload_files, ).follow(status=200) - return response def helper_check_final_grade_upload(self, course, expected_number_of_emails): response = self.helper_upload_grades(course, final_grades=True) diff --git a/evap/locale/de/formats.py b/evap/locale/de/formats.py index 42ad0f8e57..8a74b1ce39 100644 --- a/evap/locale/de/formats.py +++ b/evap/locale/de/formats.py @@ -1 +1 @@ -from evap.locale.en.formats import * # pylint: disable=unused-wildcard-import,wildcard-import +from evap.locale.en.formats import * # noqa: F403 diff --git a/evap/results/exporters.py b/evap/results/exporters.py index 4a4dba8e3e..e054c37cb5 100644 --- a/evap/results/exporters.py +++ b/evap/results/exporters.py @@ -72,11 +72,12 @@ def init_grade_styles(cls): "This can happen, if the file is imported / run multiple " "times in one application run.", ImportWarning, + stacklevel=3, ) return grade_base_style = "pattern: pattern solid, fore_colour {}; alignment: horiz centre; font: bold on; borders: left medium, right medium" - for i in range(0, cls.NUM_GRADE_COLORS): + for i in range(cls.NUM_GRADE_COLORS): grade = 1 + i * cls.STEP color = get_grade_color(grade) palette_index = cls.CUSTOM_COLOR_START + i @@ -226,11 +227,13 @@ def write_overall_results(self, evaluations_with_results, course_results_exist): ) self.write_cell(_("Evaluation weight"), "bold") - weight_percentages = (f"{e.weight_percentage}%" if gt1 else None for e, gt1 in zip(evaluations, count_gt_1)) + weight_percentages = ( + f"{e.weight_percentage}%" if gt1 else None for e, gt1 in zip(evaluations, count_gt_1, strict=True) + ) self.write_row(weight_percentages, lambda s: "evaluation_weight" if s is not None else "default") self.write_cell(_("Course Grade"), "bold") - for evaluation, gt1 in zip(evaluations, count_gt_1): + for evaluation, gt1 in zip(evaluations, count_gt_1, strict=True): if not gt1: self.write_cell() continue @@ -377,5 +380,5 @@ def export_impl(self): # pylint: disable=arguments-differ question_title = (f"{contributor_name}: " if contributor_name else "") + question.text first_col = chain([question_title], repeat("")) - for answer, first_cell, line_style in zip(answers, first_col, line_styles): + for answer, first_cell, line_style in zip(answers, first_col, line_styles, strict=False): self.write_row([first_cell, answer], style=line_style) diff --git a/evap/results/templatetags/results_templatetags.py b/evap/results/templatetags/results_templatetags.py index 23f35a4a4c..38d9f41162 100644 --- a/evap/results/templatetags/results_templatetags.py +++ b/evap/results/templatetags/results_templatetags.py @@ -1,4 +1,5 @@ -from typing import Any, Iterable +from collections.abc import Iterable +from typing import Any from django.template import Library diff --git a/evap/results/tests/test_exporters.py b/evap/results/tests/test_exporters.py index c5779ea3bb..5e6dad0230 100644 --- a/evap/results/tests/test_exporters.py +++ b/evap/results/tests/test_exporters.py @@ -398,9 +398,7 @@ def test_multiple_evaluations(self): sheet = self.get_export_sheet(semester, degree, [evaluation1.course.type.id, evaluation2.course.type.id]) - self.assertEqual( - set(sheet.row_values(0)[1:]), set((evaluation1.full_name + "\n", evaluation2.full_name + "\n")) - ) + self.assertEqual(set(sheet.row_values(0)[1:]), {evaluation1.full_name + "\n", evaluation2.full_name + "\n"}) def test_correct_grades_and_bottom_numbers(self): degree = baker.make(Degree) @@ -449,7 +447,7 @@ def test_course_grade(self): questionnaire = baker.make(Questionnaire) question = baker.make(Question, type=QuestionType.POSITIVE_LIKERT, questionnaire=questionnaire) - for grades, e in zip(grades_per_eval, evaluations): + for grades, e in zip(grades_per_eval, evaluations, strict=True): make_rating_answer_counters(question, e.general_contribution, grades) e.general_contribution.questionnaires.set([questionnaire]) for evaluation in evaluations: diff --git a/evap/results/tools.py b/evap/results/tools.py index 52084fbe2e..0a9f045041 100644 --- a/evap/results/tools.py +++ b/evap/results/tools.py @@ -103,7 +103,9 @@ def approval_count(self) -> int: class AnsweredRatingResult(PublishedRatingResult): @property def average(self) -> float: - return sum(grade * count for count, grade in zip(self.counts, self.choices.grades)) / self.count_sum + return ( + sum(grade * count for count, grade in zip(self.counts, self.choices.grades, strict=True)) / self.count_sum + ) class TextResult: @@ -288,7 +290,7 @@ def unipolarized_distribution(result): if not result.counts: return None - for counts, grade in zip(result.counts, result.choices.grades): + for counts, grade in zip(result.counts, result.choices.grades, strict=True): grade_fraction, grade = modf(grade) grade = int(grade) summed_distribution[grade - 1] += (1 - grade_fraction) * counts @@ -469,13 +471,12 @@ def textanswers_visible_to(contribution): return TextAnswerVisibility(visible_by_contribution=sorted_contributors, visible_by_delegation_count=num_delegates) -def can_textanswer_be_seen_by( +def can_textanswer_be_seen_by( # noqa: PLR0911 user: UserProfile, represented_users: list[UserProfile], textanswer: TextAnswer, view: str, ) -> bool: - # pylint: disable=too-many-return-statements assert textanswer.review_decision in [TextAnswer.ReviewDecision.PRIVATE, TextAnswer.ReviewDecision.PUBLIC] contributor = textanswer.contribution.contributor diff --git a/evap/results/views.py b/evap/results/views.py index 49d7ee4a9e..5501199442 100644 --- a/evap/results/views.py +++ b/evap/results/views.py @@ -106,7 +106,7 @@ def update_template_cache_of_published_evaluations_in_course(course): def get_evaluations_with_prefetched_data(evaluations): - if isinstance(evaluations, QuerySet): # type: ignore + if isinstance(evaluations, QuerySet): # type: ignore[misc] evaluations = evaluations.select_related("course__type").prefetch_related( "course__degrees", "course__semester", @@ -148,7 +148,7 @@ def index(request): annotated_courses = ( Course.objects.filter(pk__in=course_pks).annotate(num_evaluations=Count("evaluations")).order_by("pk").defer() ) - for course, annotated_course in zip(courses_and_evaluations.keys(), annotated_courses): + for course, annotated_course in zip(courses_and_evaluations.keys(), annotated_courses, strict=True): course.num_evaluations = annotated_course.num_evaluations degrees = Degree.objects.filter(courses__pk__in=course_pks).distinct() diff --git a/evap/rewards/tools.py b/evap/rewards/tools.py index 142925b066..7e5bae5fba 100644 --- a/evap/rewards/tools.py +++ b/evap/rewards/tools.py @@ -31,22 +31,22 @@ def save_redemptions(request, redemptions: dict[int, int], previous_redeemed_poi # check consistent previous redeemed points # do not validate reward points, to allow receiving points after page load if previous_redeemed_points != redeemed_points_of_user(request.user): - raise OutdatedRedemptionDataError() + raise OutdatedRedemptionDataError total_points_available = reward_points_of_user(request.user) total_points_redeemed = sum(redemptions.values()) if total_points_redeemed <= 0: - raise NoPointsSelectedError() + raise NoPointsSelectedError if total_points_redeemed > total_points_available: - raise NotEnoughPointsError() + raise NotEnoughPointsError for event_id in redemptions: if redemptions[event_id] > 0: event = get_object_or_404(RewardPointRedemptionEvent, pk=event_id) if event.redeem_end_date < date.today(): - raise RedemptionEventExpiredError() + raise RedemptionEventExpiredError RewardPointRedemption.objects.create(user_profile=request.user, value=redemptions[event_id], event=event) diff --git a/evap/rewards/views.py b/evap/rewards/views.py index 42568de6aa..7a727f546c 100644 --- a/evap/rewards/views.py +++ b/evap/rewards/views.py @@ -78,15 +78,18 @@ def index(request): reward_point_redemptions = RewardPointRedemption.objects.filter(user_profile=request.user) events = RewardPointRedemptionEvent.objects.filter(redeem_end_date__gte=datetime.now()).order_by("date") - reward_point_actions = [] - for granting in reward_point_grantings: - reward_point_actions.append( - (granting.granting_time, _("Reward for") + " " + granting.semester.name, granting.value, "") - ) - for redemption in reward_point_redemptions: - reward_point_actions.append((redemption.redemption_time, redemption.event.name, "", redemption.value)) - - reward_point_actions.sort(key=lambda action: action[0], reverse=True) + granted_point_actions = [ + (granting.granting_time, _("Reward for") + " " + granting.semester.name, granting.value, "") + for granting in reward_point_grantings + ] + redemption_point_actions = [ + (redemption.redemption_time, redemption.event.name, "", redemption.value) + for redemption in reward_point_redemptions + ] + + reward_point_actions = sorted( + granted_point_actions + redemption_point_actions, key=lambda action: action[0], reverse=True + ) template_data = { "reward_point_actions": reward_point_actions, diff --git a/evap/settings.py b/evap/settings.py index 4164accddc..d23b52843f 100644 --- a/evap/settings.py +++ b/evap/settings.py @@ -408,12 +408,10 @@ def CHARACTER_ALLOWED_IN_NAME(character): # pylint: disable=invalid-name # Create a localsettings.py if you want to locally override settings # and don't want the changes to appear in 'git status'. try: - # if a localsettings file exists (vagrant), this will cause wildcard-import errors - # if it does not, (GitHub), it would cause useless-suppression - # pylint: disable=unused-wildcard-import,wildcard-import,useless-suppression + # localsettings file may (vagrant) or may not (CI run) exist # the import can overwrite locals with a slightly different type (e.g. DATABASES), which is fine. - from evap.localsettings import * # type: ignore + from evap.localsettings import * # type: ignore # noqa: F403,PGH003 except ImportError: pass diff --git a/evap/staff/fixtures/__init__.py b/evap/staff/fixtures/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/evap/staff/fixtures/excel_files_test_data.py b/evap/staff/fixtures/excel_files_test_data.py index 71f33f9c15..0df69c6c1c 100644 --- a/evap/staff/fixtures/excel_files_test_data.py +++ b/evap/staff/fixtures/excel_files_test_data.py @@ -208,7 +208,7 @@ ['Prof. Dr.', 'Prorsus', 'Christoph', '123@external.com'] ] -random_file_content = b"Hallo Welt\n" +random_file_content = b"Hallo Welt\n" # pylint: disable=invalid-name wrong_column_count_excel_data = { diff --git a/evap/staff/forms.py b/evap/staff/forms.py index 810feeda6a..1a067e5ff4 100644 --- a/evap/staff/forms.py +++ b/evap/staff/forms.py @@ -260,7 +260,7 @@ def validate_unique(self): self.add_error(name_field, e) -class CourseForm(CourseFormMixin, forms.ModelForm): # type: ignore +class CourseForm(CourseFormMixin, forms.ModelForm): # type: ignore[misc] semester = forms.ModelChoiceField(Semester.objects.all(), disabled=True, required=False, widget=forms.HiddenInput()) def __init__(self, *args, **kwargs): @@ -270,7 +270,7 @@ def __init__(self, *args, **kwargs): disable_all_fields(self) -class CourseCopyForm(CourseFormMixin, forms.ModelForm): # type: ignore +class CourseCopyForm(CourseFormMixin, forms.ModelForm): # type: ignore[misc] semester = forms.ModelChoiceField(Semester.objects.all()) vote_start_datetime = forms.DateTimeField(label=_("Start of evaluations"), localize=True) vote_end_date = forms.DateField(label=_("Last day of evaluations"), localize=True) @@ -683,7 +683,7 @@ def email_addresses(self): recipients = self.template.recipient_list_for_evaluation( self.evaluation, self.recipient_groups, filter_users_in_cc=False ) - return set(user.email for user in recipients if user.email) + return {user.email for user in recipients if user.email} def send(self, request): self.template.subject = self.cleaned_data.get("subject") @@ -817,7 +817,7 @@ def handle_moved_contributors(data, **kwargs): evaluation = None total_forms = int(data["contributions-TOTAL_FORMS"]) - for i in range(0, total_forms): + for i in range(total_forms): prefix = "contributions-" + str(i) + "-" current_id = data.get(prefix + "id", "") contributor = data.get(prefix + "contributor", "") @@ -833,7 +833,7 @@ def handle_moved_contributors(data, **kwargs): continue # find the form with that previous contribution and then swap the contributions - for j in range(0, total_forms): + for j in range(total_forms): other_prefix = "contributions-" + str(j) + "-" other_id = data[other_prefix + "id"] if other_id == previous_id: diff --git a/evap/staff/importers/__init__.py b/evap/staff/importers/__init__.py index bc58f53589..dac77ea487 100644 --- a/evap/staff/importers/__init__.py +++ b/evap/staff/importers/__init__.py @@ -2,3 +2,12 @@ from .enrollment import import_enrollments from .person import import_persons_from_evaluation, import_persons_from_file from .user import import_users + +__all__ = [ + "ImporterLog", + "ImporterLogEntry", + "import_enrollments", + "import_persons_from_evaluation", + "import_persons_from_file", + "import_users", +] diff --git a/evap/staff/importers/base.py b/evap/staff/importers/base.py index 2c942ec7a9..d4b7668402 100644 --- a/evap/staff/importers/base.py +++ b/evap/staff/importers/base.py @@ -179,9 +179,9 @@ class InputRow(ABC): # MyPy is currently broken with abstract properties: https://github.com/python/mypy/issues/8996 column_count: int + @abstractmethod def __init__(self, location: ExcelFileLocation, *cells: Iterable[str]): - # this can be an abstractmethod from python3.10 on, before it doesn't work with the derived dataclasses - raise NotImplementedError # pragma: no cover + pass @classmethod def from_cells(cls, location: ExcelFileLocation, cells: Iterable[str]): @@ -201,14 +201,14 @@ def __init__(self, skip_first_n_rows: int, row_cls: type[InputRow], importer_log def map(self, file_content: bytes): try: book = openpyxl.load_workbook(BytesIO(file_content)) - except Exception as e: + except Exception as e: # noqa: BLE001 raise ImporterError( message=_("Couldn't read the file. Error: {}").format(e), category=ImporterLogEntry.Category.SCHEMA, ) from e rows = [] - for sheet in book: # type: ignore + for sheet in book: # type: ignore[attr-defined] if sheet.max_row <= self.skip_first_n_rows: continue diff --git a/evap/staff/importers/enrollment.py b/evap/staff/importers/enrollment.py index a3ab000d3c..8c171dad6f 100644 --- a/evap/staff/importers/enrollment.py +++ b/evap/staff/importers/enrollment.py @@ -377,10 +377,10 @@ def set_course_merge_target(self, course_data: CourseData) -> None: if course_with_same_name_en != course_with_same_name_de: if course_with_same_name_en is not None: - raise self.NameEnCollisionError() + raise self.NameEnCollisionError if course_with_same_name_de is not None: - raise self.NameDeCollisionError() + raise self.NameDeCollisionError assert course_with_same_name_en is not None assert course_with_same_name_de is not None @@ -445,7 +445,7 @@ def finalize(self) -> None: self.importer_log.add_warning( _( 'Course "{course_name}" already exists. The course will not be created, instead users are imported into the ' - + "evaluation of the existing course and any additional degrees are added." + "evaluation of the existing course and any additional degrees are added.", ).format(course_name=name_en), category=ImporterLogEntry.Category.EXISTS, ) @@ -559,7 +559,7 @@ def finalize(self) -> None: seen_evaluation_names = self.participant_emails_per_course_name_en.keys() existing_participation_pairs = [ - (participation.evaluation.course.name_en, participation.userprofile.email) # type: ignore + (participation.evaluation.course.name_en, participation.userprofile.email) # type: ignore[misc] for participation in Evaluation.participants.through._default_manager.filter( evaluation__course__name_en__in=seen_evaluation_names, userprofile__email__in=seen_user_emails ).prefetch_related("userprofile", "evaluation__course") diff --git a/evap/staff/staff_mode.py b/evap/staff/staff_mode.py index ead425d0d3..0ba8300ba7 100644 --- a/evap/staff/staff_mode.py +++ b/evap/staff/staff_mode.py @@ -42,8 +42,7 @@ def middleware(request): request.user.is_manager = False request.user.is_reviewer = False - response = get_response(request) - return response + return get_response(request) return middleware diff --git a/evap/staff/tests/test_importers.py b/evap/staff/tests/test_importers.py index 25e5e7a66f..e9330c61b7 100644 --- a/evap/staff/tests/test_importers.py +++ b/evap/staff/tests/test_importers.py @@ -665,7 +665,7 @@ def test_existing_course_is_not_recreated(self): self.assertIn( 'Course "Shake" already exists. The course will not be created, instead users are imported into the ' - + "evaluation of the existing course and any additional degrees are added.", + "evaluation of the existing course and any additional degrees are added.", warnings_test, ) self.assertListEqual(warnings_test, warnings_notest) @@ -716,9 +716,9 @@ def test_existing_course_different_attributes(self): importer_log, ImporterLogEntry.Category.COURSE, "Sheet "BA Belegungen", row 2 and 1 other place: Course "Shake" already exists in this " - + "semester, but the courses can not be merged for the following reasons:" - + "
- the course type does not match" - + "
- the responsibles of the course do not match", + "semester, but the courses can not be merged for the following reasons:" + "
- the course type does not match" + "
- the responsibles of the course do not match", ) def test_existing_course_with_published_evaluation(self): @@ -737,8 +737,8 @@ def test_existing_course_with_published_evaluation(self): importer_log, ImporterLogEntry.Category.COURSE, "Sheet "BA Belegungen", row 2 and 1 other place: " - + "Course "Shake" already exists in this semester, but the courses can not be merged for the following reasons:
" - + "- the import would add participants to the existing evaluation but the evaluation is already running", + "Course "Shake" already exists in this semester, but the courses can not be merged for the following reasons:
" + "- the import would add participants to the existing evaluation but the evaluation is already running", ) # Attempt with earlier state but set _participant_count @@ -772,8 +772,8 @@ def test_existing_course_with_single_result(self): importer_log, ImporterLogEntry.Category.COURSE, "Sheet "BA Belegungen", row 2 and 1 other place: " - + "Course "Shake" already exists in this semester, but the courses can not be merged for the following reasons:
" - + "- the evaluation of the existing course is a single result", + "Course "Shake" already exists in this semester, but the courses can not be merged for the following reasons:
" + "- the evaluation of the existing course is a single result", ) def test_existing_course_equal_except_evaluations(self): @@ -790,8 +790,8 @@ def test_existing_course_equal_except_evaluations(self): importer_log, ImporterLogEntry.Category.COURSE, "Sheet "BA Belegungen", row 2 and 1 other place: Course "Shake" already exists in " - + "this semester, but the courses can not be merged for the following reasons:" - + "
- the existing course does not have exactly one evaluation", + "this semester, but the courses can not be merged for the following reasons:" + "
- the existing course does not have exactly one evaluation", ) def test_existing_course_different_grading(self): @@ -809,8 +809,8 @@ def test_existing_course_different_grading(self): importer_log, ImporterLogEntry.Category.COURSE, "Sheet "BA Belegungen", row 2 and 1 other place: Course "Shake" already exists in this " - + "semester, but the courses can not be merged for the following reasons:" - + "
- the evaluation of the existing course has a mismatching grading specification", + "semester, but the courses can not be merged for the following reasons:" + "
- the evaluation of the existing course has a mismatching grading specification", ) def test_wrong_column_count(self): @@ -983,7 +983,7 @@ def test_import_new_contributor(self): self.assertEqual(self.evaluation1.contributions.count(), 3) self.assertEqual( set(UserProfile.objects.filter(contributions__evaluation=self.evaluation1)), - set([self.contributor1, self.contributor2]), + {self.contributor1, self.contributor2}, ) def test_import_existing_participant(self): @@ -1032,7 +1032,7 @@ def test_import_new_participant(self): self.assertIn(f"{self.participant2.email}", "".join(success_messages)) self.assertEqual(self.evaluation1.participants.count(), 2) - self.assertEqual(set(self.evaluation1.participants.all()), set([self.participant1, self.participant2])) + self.assertEqual(set(self.evaluation1.participants.all()), {self.participant1, self.participant2}) def test_imported_participants_are_made_active(self): self.participant2.is_active = False diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index c8e057b6b7..27842c8d41 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -93,11 +93,11 @@ def test_merge_handles_all_attributes(self): user1 = baker.make(UserProfile) user2 = baker.make(UserProfile) - all_attrs = list(field.name for field in UserProfile._meta.get_fields(include_hidden=True)) + all_attrs = [field.name for field in UserProfile._meta.get_fields(include_hidden=True)] # these are relations to intermediate models generated by django for m2m relations. # we can safely ignore these since the "normal" fields of the m2m relations are present as well. - all_attrs = list(attr for attr in all_attrs if not attr.startswith("UserProfile_")) + all_attrs = [attr for attr in all_attrs if not attr.startswith("UserProfile_")] # equally named fields are not supported, sorry self.assertEqual(len(all_attrs), len(set(all_attrs))) diff --git a/evap/staff/tests/test_views.py b/evap/staff/tests/test_views.py index 3134bd0cee..f1e01cc854 100644 --- a/evap/staff/tests/test_views.py +++ b/evap/staff/tests/test_views.py @@ -1397,8 +1397,8 @@ def test_view_downloads_csv_file(self): ) expected_content = ( "Evaluation id;Course type;Course degrees;Vote end date;Timestamp\n" - + f"{self.evaluation_id};Type;;{self.vote_end_date};{self.timestamp_time}\n" - ).encode("utf-8") + f"{self.evaluation_id};Type;;{self.vote_end_date};{self.timestamp_time}\n" + ).encode() self.assertEqual(response.content, expected_content) @@ -2757,7 +2757,7 @@ def test_suggested_evaluation_ordering(self): _quantity=2, ) - for evaluation, answer_count in zip(evaluations, [1, 2]): + for evaluation, answer_count in zip(evaluations, [1, 2], strict=True): contribution = baker.make(Contribution, evaluation=evaluation, _fill_optional=["contributor"]) baker.make(TextAnswer, contribution=contribution, question__type=QuestionType.TEXT, _quantity=answer_count) @@ -3695,16 +3695,16 @@ def test_questionnaire_assignment(self): self.assertEqual( set(self.evaluation_1.general_contribution.questionnaires.all()), - set([self.questionnaire_1, self.questionnaire_2]), + {self.questionnaire_1, self.questionnaire_2}, ) - self.assertEqual(set(self.evaluation_2.general_contribution.questionnaires.all()), set([self.questionnaire_2])) + self.assertEqual(set(self.evaluation_2.general_contribution.questionnaires.all()), {self.questionnaire_2}) self.assertEqual( set(self.evaluation_1.contributions.get(contributor=self.responsible).questionnaires.all()), - set([self.questionnaire_responsible]), + {self.questionnaire_responsible}, ) self.assertEqual( set(self.evaluation_2.contributions.get(contributor=self.responsible).questionnaires.all()), - set([self.questionnaire_responsible]), + {self.questionnaire_responsible}, ) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index 07cebcf411..ac289daa54 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -100,7 +100,7 @@ def find_matching_internal_user_for_email(request, email): return matching_users[0] -def bulk_update_users(request, user_file_content, test_run): +def bulk_update_users(request, user_file_content, test_run): # noqa: PLR0912 # pylint: disable=too-many-branches,too-many-locals # user_file must have one user per line in the format "{username},{email}" imported_emails = {clean_email(line.decode().split(",")[1]) for line in user_file_content.splitlines()} @@ -206,9 +206,8 @@ def bulk_update_users(request, user_file_content, test_run): for user, email in users_to_be_updated: user.email = email user.save() - userprofiles_to_create = [] - for email in emails_of_users_to_be_created: - userprofiles_to_create.append(UserProfile(email=email)) + userprofiles_to_create = [UserProfile(email=email) for email in emails_of_users_to_be_created] + UserProfile.objects.bulk_create(userprofiles_to_create) messages.success(request, _("Users have been successfully updated.")) @@ -216,10 +215,10 @@ def bulk_update_users(request, user_file_content, test_run): @transaction.atomic -def merge_users(main_user, other_user, preview=False): +def merge_users( # noqa: PLR0915 # This is much stuff to do. However, splitting it up into subtasks doesn't make much sense. + main_user, other_user, preview=False +): """Merges other_user into main_user""" - # This is much stuff to do. However, splitting it up into subtasks doesn't make much sense. - # pylint: disable=too-many-statements merged_user = {} merged_user["is_active"] = main_user.is_active or other_user.is_active @@ -228,7 +227,7 @@ def merge_users(main_user, other_user, preview=False): merged_user["first_name_given"] = main_user.first_name_given or other_user.first_name_given or "" merged_user["last_name"] = main_user.last_name or other_user.last_name or "" merged_user["email"] = main_user.email or other_user.email or None - merged_user["notes"] = "\n".join((main_user.notes, other_user.notes)).strip() + merged_user["notes"] = f"{main_user.notes}\n{other_user.notes}".strip() merged_user["groups"] = Group.objects.filter(user__in=[main_user, other_user]).distinct() merged_user["is_superuser"] = main_user.is_superuser or other_user.is_superuser diff --git a/evap/staff/views.py b/evap/staff/views.py index 733e86e5b2..601806a923 100644 --- a/evap/staff/views.py +++ b/evap/staff/views.py @@ -187,9 +187,7 @@ def get_evaluations_with_prefetched_data(semester): ) ).order_by("pk") evaluations = annotate_evaluations_with_grade_document_counts(evaluations) - evaluations = Evaluation.annotate_with_participant_and_voter_counts(evaluations) - - return evaluations + return Evaluation.annotate_with_participant_and_voter_counts(evaluations) @reviewer_required @@ -705,9 +703,9 @@ def semester_export(request, semester_id): if formset.is_valid(): include_not_enough_voters = request.POST.get("include_not_enough_voters") == "on" include_unpublished = request.POST.get("include_unpublished") == "on" - selection_list = [] - for form in formset: - selection_list.append((form.cleaned_data["selected_degrees"], form.cleaned_data["selected_course_types"])) + selection_list = [ + (form.cleaned_data["selected_degrees"], form.cleaned_data["selected_course_types"]) for form in formset + ] filename = f"Evaluation-{semester.name}-{get_language()}.xls" response = AttachmentResponse(filename, content_type="application/vnd.ms-excel") @@ -2293,7 +2291,7 @@ def user_bulk_update(request): success = False try: success = bulk_update_users(request, file_content, test_run) - except Exception: # pylint: disable=broad-except + except Exception: # noqa: BLE001 if settings.DEBUG: raise messages.error( diff --git a/evap/student/forms.py b/evap/student/forms.py index 459668c175..dccd9f6237 100644 --- a/evap/student/forms.py +++ b/evap/student/forms.py @@ -60,7 +60,7 @@ def deconstruct(self): def from_question(cls, question: Question): return cls( widget_choices=CHOICES[question.type], - choices=zip(CHOICES[question.type].values, CHOICES[question.type].names), + choices=zip(CHOICES[question.type].values, CHOICES[question.type].names, strict=True), label=question.text, allows_textanswer=question.allows_additional_textanswers, ) diff --git a/pyproject.toml b/pyproject.toml index 74a4b5cbe3..65206995f0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -22,6 +22,37 @@ extend_skip_glob = ["**/migrations/*"] ############################################## +[tool.ruff] +target-version = "py310" +line-length = 120 + +[tool.ruff.lint] +# Those are interesting, but not yet addressed: PYI,Q,SIM,PTH,TRY,RUF +select = ["F", "E", "B", "W", "N", "UP", "YTT", "FIX", "ASYNC", "A", "DJ", "EXE", "ICN", "G", "SLOT", "TID", "TCH", "INT", "C4", "ISC", "INP", "PIE", "RSE", "RET", "COM","PGH","FLY", "PERF", "PL", "BLE"] +ignore = [ + "E501", # line-too-long: black does code formatting for us + "FIX004", # hacks should be possible + "A003", # we shadow some builtins sometime + "DJ008", # do not use __str__ where it is not required + "COM812", # incompatible with formatter + "N802", # not as mighty as pylint's invalid-name https://github.com/astral-sh/ruff/issues/7660 + "PLR0913", # we can't determine a good limit for arguments. reviews should spot bad cases of this. + "PLR2004", "PLW2901" +] + +ignore-init-module-imports = true +pep8-naming.extend-ignore-names = ["assert*", "*Formset"] # custom assert methods use camelCase; Formsets use PascalCase + +[tool.ruff.lint.per-file-ignores] +"**/migrations/*.py" = ["N806"] # migrations have model classes as local variables, we use PascalCase for these +"deployment/*.py" = ["INP001"] # not intended for direct import + +[tool.ruff.format] +exclude = ["**/urls.py", "**/migrations/*.py"] + +############################################## + + [tool.pylint.master] jobs = 0 @@ -50,7 +81,6 @@ enable = ["all"] disable = [ "locally-disabled", # we allow locally disabling some checks if we think it makes sense to do that. "suppressed-message", - "line-too-long", # black does code formatting for us "ungrouped-imports", # isort "wrong-import-order", # isort "too-many-public-methods", # reported for some models, that won't change @@ -61,12 +91,13 @@ disable = [ "missing-docstring", # yeah... we don't have those "protected-access", # for us that means "be careful", not "don't do it" "too-many-lines", # we don't currently think that splitting up views.py or test_views.py creates any value - "too-many-arguments", # we can't determine a good limit here. reviews should spot bad cases of this. "duplicate-code", # Mostly imports and test setup. "cyclic-import", # We use these inside methods that require models from multiple apps. Tests will catch actual errors. "unsupported-binary-operation", # broken in pylint: https://github.com/PyCQA/pylint/issues/7381 "use-implicit-booleaness-not-comparison-to-string", # forces us to use less expressive code "use-implicit-booleaness-not-comparison-to-zero", # forces us to use less expressive code + # the following are covered by ruff + "broad-exception-caught", "line-too-long","unused-wildcard-import", "wildcard-import","too-many-arguments", "too-many-statements", "too-many-return-statements" ] ############################################## diff --git a/requirements-dev.txt b/requirements-dev.txt index 1ef1e5b0bb..dc456c8138 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -10,6 +10,7 @@ mypy~=1.9.0 openpyxl-stubs~=0.1.25 pylint-django~=2.5.4 pylint~=3.1.0 +ruff==0.3.5 tblib~=3.0.0 xlrd~=2.0.1 typeguard~=4.2.1 diff --git a/tools/enrollment_preprocessor.py b/tools/enrollment_preprocessor.py index 114eaec06a..df459cab13 100755 --- a/tools/enrollment_preprocessor.py +++ b/tools/enrollment_preprocessor.py @@ -4,11 +4,12 @@ import sys from argparse import ArgumentParser from collections import defaultdict +from collections.abc import Iterator from datetime import datetime from io import BytesIO from itertools import chain from pathlib import Path -from typing import Iterator, NamedTuple, TextIO +from typing import NamedTuple, TextIO from openpyxl import Workbook, load_workbook from openpyxl.cell import Cell