Skip to content

refactor: base PR for removal of kwargs from generators module #1785

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

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

hetangmodi-crest
Copy link
Contributor

@hetangmodi-crest hetangmodi-crest commented Jun 9, 2025

Issue number: ADDON-81314

PR Type

What kind of change does this PR introduce?

  • Feature
  • Bug Fix
  • Refactoring (no functional or API changes)
  • Documentation Update
  • Maintenance (dependency updates, CI, etc.)

Summary

Changes

This PR contains base changes for removal of kwargs from generators module.

PS: It also contains unit test changes for all files of generators module to make the pipeline green.

User experience

No change in User experience.

Checklist

If an item doesn't apply to your changes, leave it unchecked.

Review

  • self-review - I have performed a self-review of this change according to the development guidelines
  • Changes are documented. The documentation is understandable, examples work (more info)
  • PR title and description follows the contributing principles
  • meeting - I have scheduled a meeting or recorded a demo to explain these changes (if there is a video, put a link below and in the ticket)

Tests

See the testing doc.

  • Unit - tests have been added/modified to cover the changes
  • Smoke - tests have been added/modified to cover the changes
  • UI - tests have been added/modified to cover the changes
  • coverage - I have checked the code coverage of my changes (see more)

Demo/meeting:

Reviewers are encouraged to request meetings or demos if any part of the change is unclear

if item.file_name == "app.conf":
# pass kwargs only for AppConf generator
file_details = item.file_class(
global_config, input_dir, output_dir, **kwargs
Copy link
Contributor

Choose a reason for hiding this comment

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

I think **kwargs can be deleted as the init does not expects it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the changes.

Comment on lines 32 to 36
**kwargs: Any
):
self.description = kwargs["app_manifest"].get_description()
self.title = kwargs["app_manifest"].get_title()
self.author = kwargs["app_manifest"].get_authors()[0]["name"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also remove kwargs here and provide app_manifest directly? From what I see, the only initialization is in the commands/build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the changes, moved _get_app_manifest function in utils.py

self.title = kwargs["app_manifest"].get_title()
self.author = kwargs["app_manifest"].get_authors()[0]["name"]
self.addon_version = self._global_config.version
self.is_visible = str(self._global_config.has_pages()).lower()
Copy link
Contributor

Choose a reason for hiding this comment

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

previously we had global_config.meta.get("isVisible", True), can we replace it with just .has_pages()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the code.

def test_set_attributes_check_for_updates_false(
mock_app_manifest,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would change it's name to avoid naming conflicts. Same for the rest below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed the fixture.

Copy link

Code Coverage 🎉

Type PR Develop Change Status
Line Coverage 88.31% 88.27% 0.05% 🟢 Increased
Branch Coverage 81.91% 81.93% 0.03% 🔴 Decreased

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Issue to track refactor work size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants