Refactor setup.py #115

jupadin · 2023-07-21T07:09:41Z

Change Types

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Description

Improved the youth detection of leagues for '-F-' or '-M-' (see models.py:69)
Handling the resulting exception thrown by this (see setup.py:202)
Added logic to skip 'Freundschaftsspiele' (see setup.py:181)
Removed duplicate associations due to url filtering and improved url concatenation (see parsing.py:24)

Bugfix 1
Could not initialize app due to missing games for season 2022 / 2023 and name comparison (see models.py:45).
Fixed by checking
- what did not work before?
- what happens now instead?
Steps to reproduce
1. Call setup and check whether data is ingested.

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read CONTRIBUTING.md.
I have added tests to cover my changes.
My tests run automatically.
All new and existing tests passed.

…rate files.

sonarcloud · 2023-07-21T07:10:08Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

djbrown

The PR title is kinda misleading:
it does certainly contain refactorings (keeping the observable behavior of the software),
but also includes functional changes (league filtering) and loggin changes.

Also I thoroughly reviewed the diffs and commented things aspects to reconsider/improve.
Overall looks good to me though. Yet my top priority now would be to fix the association scraping (e.g. via #77)

djbrown · 2023-07-28T21:51:03Z

src/leagues/management/commands/setup.py

+    try: 
+        if League.is_youth(abbreviation, name) and not options['youth']:
+            logger.debug('SKIPPING League (youth league): %s %s %s', bhv_id, abbreviation, name)
+            return
+    except YouthUndecidableError:
+        logger.info(3*'\t' + 'Youth undecidable for league: %s %s %s', bhv_id, abbreviation, name)


Which benefit do you see over the current exception handling?
With logger.exception(str) the same log output would be produced, including the current exception message and stacktrace.

djbrown · 2023-07-28T22:09:12Z

src/leagues/management/commands/setup.py

-    try:
-        abbreviation = Association.get_association_abbreviation(name)
-    except KeyError:
-        LOGGER.warning("No abbreviation for association '%s'", name)


I'm fine with restructuring this exception handling to a None-check.
I think the warning should still be logged though.

djbrown · 2023-07-28T22:14:51Z

src/base/parsing.py

+
+    # Filter and return all striped association URLs
+    #return [item if item.startswith('http') else root_url + item for item in items]
+    return [root_url + item.lstrip('/') for item in items if not item.startswith('https')]


how does this improve URL parsing?
can you add a test for this, which would fail without this change?

There have been some duplicate entries (abbreviations) in this list. I tried to filter out those before making the HTTP-Request.

djbrown · 2023-07-28T22:19:16Z

src/hbscorez/settings.py

+        'file': {
+            'level': 'INFO',
+            'class': 'logging.FileHandler',
+            'filename': ROOT_DIR / 'hbscorez_INFO.log',
+            'formatter': 'verbose',
+            'encoding': 'UTF-8'


I'll have to check if the production settings extends or overwrite the logging configuration.
Also I'm not sure wether this should really be the default config, or if it's just personal preference.

The idea was to split the logging into different files, which retrospecively spoken, does not improve anything. However the encoding could be added due to some german "Umlaute" appearing in the association, district and / or team names. New PR is in the pipeline :)

djbrown · 2023-07-28T22:21:00Z

.gitignore

@@ -5,10 +5,11 @@
 /.vscode/
 /htmlcov/
 /logs
-/mails/
+mails/


why remove the leading slash?
this would cause any directory named mails in the whole project to be ignored.
including any module with that name. when actually we only need to ignore the root mail directory.

djbrown · 2023-07-28T22:22:09Z

.gitignore

 /media/
 /reports/
 /static/
+venv/


shouldn't be necessary when using pipenv

Did not know pipenv beforehand so to get the project up and running I just used the (for me well known) python module 'venv'.

djbrown · 2023-07-28T22:22:18Z

.gitignore

@@ -17,4 +18,5 @@ db.sqlite3
 docker-stack*.yml
 geckodriver.log
 hbscorez.log
-src/hbscorez/settings_docker_stack*.py
+hbscorez*.log
+src/hbscorez/settings_docker_stack*.py


why remove the trailing space?

djbrown · 2023-07-28T22:28:35Z

src/leagues/management/commands/setup.py

 from teams.models import Team

-LOGGER = logging.getLogger('hbscorez')
-
+logger = logging.getLogger('hbscorez')


shouldn't lowercase this module level constant, see pylint-dev/pylint#2166
also if this was to be renamed, all the other logger variables in the project should also be renamed

djbrown · 2023-07-28T22:29:26Z

src/leagues/models.py

-            'Inklusion', 'Special Olympics']
-        adult_match_strong = any(n in name for n in adult_name_indicators)
+            'Inklusion', 'Special Olympics',
+            '-F-', '-M-'


can you add a test for this?

Will do in the new PR :)

djbrown · 2023-07-28T22:39:25Z

src/leagues/models.py

+            'Inklusion', 'Special Olympics',
+            '-F-', '-M-'
+            ]
+        adult_match_strong = any(n in name for n in adult_name_indicators) and "M-Runde" not in name


this change means: don't strongly match leagues as adult, whose name contains M-Runde
it (probably unnecessarily) adds a new condition branch - and thus complexity - complexity to this evaluation statement.
couldn't we instead just solve this by adding M-Runde to youth_name_indicators_direct ?
also this should be backed by a test

Should be backed by a test - There was a corner case where M-Runde appeared (at least with the old root URL) in the youth league and is also declared as adult league due to the M in its name.

djbrown · 2023-07-28T23:00:24Z

Thanks you for the PR though! 😉 I really appreciate your work to improve the project, especially this biggest and most complex module.
The comments may seem harsh or nitpicky, but I'm just making sure I undestand the changes and only needed changed are introduced.
Also I changed the change type to from feature to bugfix, since the PR only fixes specific cases without introducing new use cases.

jupadin · 2023-08-08T11:18:03Z

Hi,

yes, might be too much changes to be declared as a PR.
If okay, we could just close this PR and I would have a look on the current branch / commit and see how I can integrate the adjustments more clearly.
In your last commit, I also saw the change of the Root-URL -:)

jupadin added 5 commits July 21, 2023 08:45

Updated gitignore for mails and virtual environments.

bc3838b

Updated gitignore for log files.

e24a814

bump.

767b621

Improved logging style, human readable console output and log to sepa…

1d6c50c

…rate files.

Improved youth detection (-s 2022 -a 78 -d 141 -l 88871 | 88876).

017a254

jupadin requested a review from djbrown as a code owner July 21, 2023 07:09

djbrown requested changes Jul 28, 2023

View reviewed changes

djbrown force-pushed the master branch 2 times, most recently from c49c030 to 013af77 Compare August 3, 2023 02:16

jupadin closed this Aug 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor setup.py #115

Refactor setup.py #115

jupadin commented Jul 21, 2023 •

edited by djbrown

Loading

sonarcloud bot commented Jul 21, 2023

djbrown left a comment

djbrown Jul 28, 2023

djbrown Jul 28, 2023

djbrown Jul 28, 2023

jupadin Aug 8, 2023

djbrown Jul 28, 2023

jupadin Aug 8, 2023

djbrown Jul 28, 2023

djbrown Jul 28, 2023

jupadin Aug 8, 2023

djbrown Jul 28, 2023

djbrown Jul 28, 2023

djbrown Jul 28, 2023

jupadin Aug 8, 2023

djbrown Jul 28, 2023

jupadin Aug 8, 2023

djbrown commented Jul 28, 2023

jupadin commented Aug 8, 2023

Refactor setup.py #115

Refactor setup.py #115

Conversation

jupadin commented Jul 21, 2023 • edited by djbrown Loading

Change Types

Description

Checklist

sonarcloud bot commented Jul 21, 2023

djbrown left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djbrown commented Jul 28, 2023

jupadin commented Aug 8, 2023

jupadin commented Jul 21, 2023 •

edited by djbrown

Loading