-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Test against package names containing periods #2524
Conversation
The test rejects any package name containing a period. Explanation: When the packages are compiled into sublime-package bundles any filenames containing a period confuse the Sublime Text plugin loader. This is on account of python (sub)modules being separated by a period. It attempts to, for example, "importlib.import_module( 'Module.app Menu')". This maps to looking for module "Module" and "app Menu" within for which the sublime-package would be Module.sublime-package but is not where our plugin is packaged to by Package Control.
Good points. I actually thought at one point that packages could still be imported using However, I don't think merging this is the right thing to do (right now), since the names with dots aguably make sense a lot and don't have an equally meaningful alternative (e.g. |
I agree that rejecting names on the basis of their including a period isn't the right thing to do. There's three options that could be looked at, then:
I think both of the Package Control-side changes will require some careful thought about what's going to happen to the archives that have already been installed by people. Assuming they're equally troublesome, I prefer the last of the three options. |
I guess I missed this. As an author of one of these said packages, We have one more option: Ask each author to rename. The list is small enough that it shouldn't be too hard to get the majority on board. |
Pinging @wbond~ |
I just spent a bunch of time trying to see if we can work around importing packages with a I'm thinking this issue should be resolve by:
We can let the individual package maintainers decide if they want to deal with the renaming issue. |
|
It might be best to just restrict the use of |
There would certainly be a lot less room for confusion. |
I'm okay with restricting package names to not include dots, but we need to rename all packages that currently contain them or rather make the developers rename these (because renaming silently is not good). I don't feel like telling them about this myself tbh. |
If @wbond gives the okay. I'll do the leg work. |
@wbond what's the verdict? |
Will a mention from this issue be enough or should we create an issue for each repo? I prefer the first, considering that you get email notifications by default. |
We still are going to merge this? Can we start counting the 14 days time out today, and after that, either close this or set those packages as @FichteFoll, I reviewed every package on the list @wbond posted and opened a issue on each repository:
Not found
Removed from the channel
Already renamed
Renaming pending
|
Updated name of jQuery Mobile 14 Snippets to not include a dot As suggested in wbond#2524
Updated package name as proposed in wbond#2524. Closes wpseek/sublime-text-2-wpseek#6
Thanks for taking the initiative for this! I'll try to keep that list updated when I merge changes to the channel. We are probably going to prune a couple missing packages at some point in the future. For this PR, we can just remove the offending and deleted packages when the others have been renamed. I'd rather give people a one month period before they respond and force-rename their packages when they haven't. The reason why declaring ST2 compatibility won't work is that I we want to test all repositories for not containing dots. |
Why not add some code - if an author includes a decimal, when PackageControl handles the installation, etc... Modify all files with import * and change the . to an _ and let the author know, and output the information in the PackageControl log.. This way, existing plugins with incorrect naming can still be used - but it requires some preprocessing of the package each time an update occurs, etc... |
While it is possible, I would say it is too much overhead to handle. I my opinion, there is not urgent need to allow packages use dots on their names, and if their authors are not available to simply rename the package, I would say the package is more likely to be unmaintained/abandoned, then, there is not much need for they to be on Package Control. |
Not worth it, imo. I say we just rename all offending packages and prune the missing ones. Not sure when I'll get to that, though. |
Could this be given more visibility ( documentation update would be great ), to give a heads-up to future package writer ? |
This is old hat by now. Nothing mentioned here is still actionable. The docs already very clearly cover invalid characters for package names and it hasn’t been an issue for years. The only real issue was the change from ST2 to ST3, but we’re at ST4 already (and for several years). Packages that stopped working have either already been fixed or abandoned. Let’s leave it at that. |
The test rejects any package name containing a period because they will error in Sublime Text 3 if they define python modules.
When the packages are compiled into sublime-package bundles any filenames containing a period confuse the Sublime Text plugin loader. This is on account of python (sub)modules being separated by a period. It attempts to, for example,
importlib.import_module('Module.app Menu')
. This maps to looking for module "Module" and "app Menu" within for which the sublime-package would be Module.sublime-package but is not where our plugin is packaged to by Package Control.I'm not sure whether this is the right way to solve this. Arguably Package Control or Sublime should handle the scenario I'm testing against here. Some examples of troublesome packages (you'll see an ImportError in the console when you load Sublime Text 3):
Currently failing tests, which do not all include python modules and therefore do not experience this error: