Skip to content

Maj/review matching new code#65

Merged
FlorianWilhelm merged 3 commits intomainfrom
maj/review_matching_new_code
Feb 18, 2026
Merged

Maj/review matching new code#65
FlorianWilhelm merged 3 commits intomainfrom
maj/review_matching_new_code

Conversation

@MedAmineGBerry
Copy link
Collaborator

Description

in this PR, I created the new code for the assignement.
we now would have a helpers file containing the functions, a notebook to execute, 2 config files and a readme.

I kept the essence of the notebook as it was, the simple and approachable process as it was built before.
I mainly moved some functions to the helpers, added the readme and created the config file.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 21393956766

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 41.091%

Totals Coverage Status
Change from base Build 20961419592: 0.0%
Covered Lines: 706
Relevant Lines: 1570

💛 - Coveralls

Copy link
Member

@FlorianWilhelm FlorianWilhelm left a comment

Choose a reason for hiding this comment

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

Really great job 🤩 The code is much clearer and I like the short and concise functions! Also great to have a README with it. I just added a few minor comments. Feel free to merge then.

Copy link
Member

Choose a reason for hiding this comment

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

Really cool idea with having all this in various configs. I just wonder if it would be better to have one file directly in the notebook folder which combines those two? Could be even easier? Just an idea. This also works and is fine.

Copy link
Member

Choose a reason for hiding this comment

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

Hm... I think precommit was configured to remove the output of all notebooks before a commit. We should do this as this file contains sensitive information like the names. Also the graphics make a huge commit.

this might have to be updated every year."""
data = load_yaml(path)
mapping: dict[str, str] = {}
for section in ['general', 'pycon', 'pydata']:
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the main topics, i.e. general, pycon and pydata, be clear from the structure of the yaml? It feels a bit like double encoding. In the yaml and here. What if there is a typo in the section of the yaml?

counts = pd.Series(scored).value_counts().reset_index()
counts.columns = [Col.submission, Col.nreviews]
else:
counts = pd.DataFrame({Col.submission: [], Col.nreviews: []})
Copy link
Member

Choose a reason for hiding this comment

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

extra empty line after counts makes it clearer.

internal_col_map[src] = getattr(Col, dst)
else:
internal_col_map[src] = dst
df = gsheet_df.rename(columns=internal_col_map).copy()
Copy link
Member

Choose a reason for hiding this comment

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

Also empty lines after the else clause and before the for. Did you run ruff format?

@FlorianWilhelm FlorianWilhelm merged commit e82c06c into main Feb 18, 2026
17 of 18 checks passed
@FlorianWilhelm FlorianWilhelm deleted the maj/review_matching_new_code branch February 18, 2026 16:21
github-actions bot pushed a commit that referenced this pull request Feb 18, 2026
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.

3 participants