Skip to content

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 a team and codegen-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?

@codecov
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.

4 participants