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

Remove from_config and only keep from_configs #659

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

PGijsbers
Copy link
Collaborator

Working towards a moving target at #658, I am splitting off individual cleaned up commits for easier reviews.

This PR removes the from_config function since it is interchangable with the more flexible from_configs.
I could detect no use for the runscores script, and it didn't even work, so I took the liberty to also remove that.

@PGijsbers PGijsbers requested a review from Innixma November 28, 2024 18:52
@codecov-commenter
Copy link

codecov-commenter commented Nov 28, 2024

Codecov Report

Attention: Patch coverage is 89.81481% with 11 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@aded7c7). Learn more about missing BASE report.

Files with missing lines Patch % Lines
amlb/resources.py 73.80% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master     #659   +/-   ##
=========================================
  Coverage          ?   70.05%           
=========================================
  Files             ?       55           
  Lines             ?     6773           
  Branches          ?        0           
=========================================
  Hits              ?     4745           
  Misses            ?     2028           
  Partials          ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator Author

@PGijsbers PGijsbers left a comment

Choose a reason for hiding this comment

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

Few actual changes, mostly sanity check and also wondering if you used runscores in any way.

Comment on lines -262 to -266
def from_config(config: Namespace):
global __INSTANCE__
transform_config(config, _backward_compatibility_config_rules_)
__INSTANCE__ = Resources(config)
return __INSTANCE__
Copy link
Collaborator Author

@PGijsbers PGijsbers Nov 29, 2024

Choose a reason for hiding this comment

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

Actual change, the replacing function is directly below (unchanged).

return from_config(
return from_configs(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Change

@PGijsbers PGijsbers requested review from eddiebergman and removed request for Innixma December 3, 2024 19:17
@PGijsbers
Copy link
Collaborator Author

@eddiebergman I was doing the "round-robin" reviewer assignment, but it seemed like Nick might be on vacation. So to keep things moving I'm assigning you instead. Hope that's alright with you.

@eddiebergman
Copy link
Collaborator

Looks good to me, thanks for highlighting the actual changes. Not familiar with this functionality really, but makes sense from a high-level overview.

One recommendation is to have a blanket ruff formatting PR to make future changes easier to review :)

@PGijsbers PGijsbers merged commit 5707f2e into master Dec 4, 2024
9 of 38 checks passed
@PGijsbers PGijsbers deleted the remove-from-config branch December 4, 2024 18:53
@PGijsbers
Copy link
Collaborator Author

Thanks for checking!

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