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

Fix: refactor gather elements #1124

Merged
merged 1 commit into from
Nov 15, 2024
Merged

Conversation

ajasnosz
Copy link
Collaborator

@ajasnosz ajasnosz commented Nov 6, 2024

Description

Refactor of gather_elements() method.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Refactor/improvement

How Has This Been Tested?

manual, unit, integration

Checklist

  • My commit message is conventional
  • I have run pre-commit on all files before creating the PR
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have checked my code and corrected any misspellings

@ajasnosz ajasnosz force-pushed the fix/refactor-gather-elements branch from 60267f5 to c9529cb Compare November 6, 2024 14:53
@ajasnosz ajasnosz marked this pull request as ready for review November 6, 2024 14:53
return active_profiles

@staticmethod
def gather_profiles_from_config_file(active_profiles):
Copy link
Contributor

@ikheifets-splunk ikheifets-splunk Nov 12, 2024

Choose a reason for hiding this comment

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

probably better would be name it merge_profiles_*, because you we have active_profiles that mutable variable and it's super unclear that we modifying it inside function and after that using it on top level.

It seems this peace of code we using on both functions (super similar) can we reuse it? And I think merge_profiles would be good name for this method.

  for key, profile in profiles.items():
         if key in active_profiles:
             if not profile.get("enabled", True):
                 celery_logger.info(f"disabling profile {key}")
                 del active_profiles[key]
             else:
                 active_profiles[key] = profile
         else:
             active_profiles[key] = profile

P.S. If dangerous to touch this code we can stay with current approach

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've extracted the common part to new method, but I wouldn't try to merge them into one

@ajasnosz ajasnosz force-pushed the fix/refactor-gather-elements branch from c9529cb to c12f451 Compare November 12, 2024 13:53
@ajasnosz ajasnosz force-pushed the fix/refactor-gather-elements branch from 207cf2e to 41d50e2 Compare November 15, 2024 09:11
@ajasnosz ajasnosz merged commit 5542e9a into develop Nov 15, 2024
12 checks passed
@ajasnosz ajasnosz deleted the fix/refactor-gather-elements branch November 15, 2024 09:18
@github-actions github-actions bot locked and limited conversation to collaborators Nov 15, 2024
@srv-rr-github-token
Copy link
Contributor

🎉 This PR is included in version 1.12.1-beta.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants