Skip to content

CG-10899: file.add_import #673

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

Merged
merged 9 commits into from
Feb 28, 2025
Merged

CG-10899: file.add_import #673

merged 9 commits into from
Feb 28, 2025

Conversation

tawsifkamal
Copy link
Contributor

file.add_import_from_string and file.add_symbol_import has now been consolidated into one method file.import which takes in Symbol | str either the string import or the Symbol that needs to be imported and will handle the logic

@tawsifkamal tawsifkamal requested review from codegen-team and a team as code owners February 26, 2025 19:45
@tawsifkamal tawsifkamal requested review from jayhack and removed request for a team February 26, 2025 19:45
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ tawsifkamal
❌ codegen-bot


codegen-bot seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@bagel897 bagel897 left a comment

Choose a reason for hiding this comment

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

Some code seems to be copied from other PRs?

@writer(commit=False)
def add_import_from_import_string(self, import_string: str) -> None:
"""Adds import to the file from a string representation of an import statement.
def add_import(self, imp: Symbol | str, *, alias: str | None = None, import_type: ImportType = ImportType.UNKNOWN, is_type_import: bool = False) -> Import | None:
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 add typing overloads to make sure the user doesn't pass the other parameters if imp is of type str

if any(import_string.strip() in imp.source for imp in self.imports):
return
# Handle Symbol imports
if isinstance(imp, str):
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 change the behavior to

  1. Convert import symbol to string (if it's a symbol)
  2. Insert the string with the same logic as the previous implementation?

Copy link

codecov bot commented Feb 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files

@tawsifkamal tawsifkamal merged commit e7c28b6 into develop Feb 28, 2025
24 of 27 checks passed
@tawsifkamal tawsifkamal deleted the tawsif/add-import branch February 28, 2025 03:01
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