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

Fixing and improving the removal of imports when using VSCode #133

Merged
merged 3 commits into from
Mar 23, 2024

Conversation

rmcginty
Copy link
Contributor

The line that I replaced was supposed to be removing the duplicate import that is required in a script if you want to develop in VSCode and have code completion, etc... It turns out the line didn't actually do anything because the replace method of a string doesn't change the string, so it needed to be reassigned to the variable, like code = code.replace(...

While fixing and testing this change, I realized that just a little more work and it could remove more combinations and iterations of the imports that might be used or suggested by VSCode, so I built and tested a regex that seems to do the trick with just about every situation I've come across while using VSCode and all the Python tools.

I'm open to comments and/or feedback, so please let me know if there are any suggestions!

@@ -3724,7 +3724,8 @@ def effect(self):

# Remove an unnecessary import that may be introduced when
# running from Visual Studio Code.
code.replace("from simpinkscr import *", "")
pattern = r"^(import (inkex|simpinkscr)|(from (inkex|simpinkscr) import.*))\r*\n*"
Copy link
Owner

Choose a reason for hiding this comment

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

Replacing each space with \s+ can catch some additional (albeit rare) cases, like from simpinkscr import *.

You might also include \s* after the ^ because an import can appear within a function and therefore indented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes. Good call. I have a new regex I'm about to update it to that I believe handles every case. I had to adjust the beginning to just look for tab and space because \s matches line feeds so it was pulling blank lines prior to the import line.

@@ -3724,7 +3724,8 @@ def effect(self):

# Remove an unnecessary import that may be introduced when
# running from Visual Studio Code.
code.replace("from simpinkscr import *", "")
pattern = r"^(import (inkex|simpinkscr)|(from (inkex|simpinkscr) import.*))\r*\n*"
code = re.sub(pattern, "", code, flags=re.MULTILINE+re.IGNORECASE)
Copy link
Owner

Choose a reason for hiding this comment

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

What's the purpose of re.IGNORECASE here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I think I've been looking at too much js. I'm relatively new to Python, so wasn't sure if the import statements were case sensitive or not. I'll remove it since it serves no purpose.

@rmcginty
Copy link
Contributor Author

See the replacement regex and let me know what you think.

@@ -3724,7 +3724,8 @@ def effect(self):

# Remove an unnecessary import that may be introduced when
# running from Visual Studio Code.
code.replace("from simpinkscr import *", "")
pattern = r"^[ \t]*(import\s+(inkex|simpinkscr).*|(from\s+(inkex|simpinkscr)\s+import).*)[\r\n]?"
Copy link
Owner

Choose a reason for hiding this comment

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

Why is the ? needed at the end? Don't you always want to consume the end-of-line character?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I was testing on a regex tool and the last line didn't have an EOL, so I didn't really think about it. I assume there is no value if someone did end a file with an import, so I will remove it for clarity.

@spakin spakin merged commit f27b157 into spakin:master Mar 23, 2024
1 check passed
@rmcginty rmcginty deleted the vscode-imports-patch branch March 26, 2024 22:30
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.

2 participants