package_control_channel icon indicating copy to clipboard operation
package_control_channel copied to clipboard

Test against package names containing periods

Open icio opened this issue 11 years ago • 19 comments

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):

  • Marked.app Menu (rename PR: #2525)
  • Mou.app Menu

Currently failing tests, which do not all include python modules and therefore do not experience this error:

$ python tests/test.py 2>&1 | grep AssertionError
AssertionError: True is not false : Package names should not contain .: 'Apiary.io Blueprint'
AssertionError: True is not false : Package names should not contain .: 'Asp.Net File Switch'
AssertionError: True is not false : Package names should not contain .: 'Backbone.Marionette'
AssertionError: True is not false : Package names should not contain .: 'Backbone.js'
AssertionError: True is not false : Package names should not contain .: 'Chaplin.js'
AssertionError: True is not false : Package names should not contain .: 'Codepad.org Paste Plugin'
AssertionError: True is not false : Package names should not contain .: 'Conky.tmLanguage'
AssertionError: True is not false : Package names should not contain .: 'Dust.js'
AssertionError: True is not false : Package names should not contain .: 'Ember.js Snippets'
AssertionError: True is not false : Package names should not contain .: 'FakeImg.pl Image Placeholder Snippet'
AssertionError: True is not false : Package names should not contain .: 'jQuery Mobile 1.4 Snippets'
AssertionError: True is not false : Package names should not contain .: 'Kohana 2.x Snippets'
AssertionError: True is not false : Package names should not contain .: 'Lazy Backbone.js'
AssertionError: True is not false : Package names should not contain .: 'Marked.app Menu'
AssertionError: True is not false : Package names should not contain .: 'Mou.app Markdown'
AssertionError: True is not false : Package names should not contain .: 'objc .strings syntax language'
AssertionError: True is not false : Package names should not contain .: 'PEG.js'
AssertionError: True is not false : Package names should not contain .: 'Paste to friendpaste.com'
AssertionError: True is not false : Package names should not contain .: 'Paste to paste.pm'
AssertionError: True is not false : Package names should not contain .: 'Placehold.it Image Tag Generator'
AssertionError: True is not false : Package names should not contain .: 'Play 2.0'
AssertionError: True is not false : Package names should not contain .: 'Require Node.js Modules Helper'
AssertionError: True is not false : Package names should not contain .: 'Ruby 1.9 Hash Converter'
AssertionError: True is not false : Package names should not contain .: 'Simple Ember.js Navigator'
AssertionError: True is not false : Package names should not contain .: 'Snipt.net'
AssertionError: True is not false : Package names should not contain .: 'SourceTree.app Menu'
AssertionError: True is not false : Package names should not contain .: 'Theme - itg.flat'
AssertionError: True is not false : Package names should not contain .: 'Three.js Autocomplete'
AssertionError: True is not false : Package names should not contain .: 'Todo.txt Syntax'
AssertionError: True is not false : Package names should not contain .: 'Underscore.js Snippets'
AssertionError: True is not false : Package names should not contain .: 'wpseek.com WordPress Function Lookup'

icio avatar Dec 22 '13 22:12 icio

Good points. I actually thought at one point that packages could still be imported using importlib if they contained a dot, but I didn't test it and it seems I was wrong.

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. jQuery Mobile 1.4 Snippets). So, what probably needs to be done is a tweak in ST's internal plugin loading mechanics to circumvent issues with dotted filenames and at least make them able to be imported via importlib, since you have to use that for names with spaces anyway.

FichteFoll avatar Dec 23 '13 15:12 FichteFoll

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:

  1. Sublime Text 3 can better handle periods in sublime-package files. There's a little more involved in this than simply looking for the correct filename, as I may have allured to in my initial post description. I'm not sure that this is the correct course of action: there'd be an unacceptable amount of pussy-footing around Python's assumptions/conventions of fully-qualified (dotted) package names elsewhere in the code, potentially.
  2. Package Control avoids using sublime-package archives for packages with periods in the name. This would be fairly easy to implement -- I understand there's already a similar mechanism where the repository contains a .no-sublime-package. (Based on skuroda's post.)
  3. Package Control creates sublime-package files with sanitised filenames. For example, "Marked.app Menu" might be stored in the file "Marked_app Menu.sublime-package". This could introduce the possibility of overlapping sublime-package filenames if not handled correctly, but there are easy strategies to avoid such problems; e.g. replace _ with __ and . with _.

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.

icio avatar Dec 23 '13 20:12 icio

I guess I missed this.

As an author of one of these said packages, Chaplin.js, I'm more than willing to rename.

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.

joneshf avatar Jan 06 '14 00:01 joneshf

  1. Yes, exactly. I think checking if the fully dotted name exists in a plugin directory and then stepping down by each dot split should be sufficient. Alternatively the other way. Possible conflicts in one way or another: com, com.org, com.org.net packages.

    I don't think this has to be supported in anything that is not a packages directory. Package developers should ensure themselves that their subpackages are importable since they will likely need them anyway.

  2. Why should the situation change if Package Control unzips the .sublime-package files? Their paths would still contain dots and remain unable to be imported.

  3. This could work, but since packages sometimes require hard references in their files to point to certain resouces (syntax files, setting file from within the menu ...), so some of the packages that we "renamed" or some if their features eventually just stopped working. Furthermore, it creates uncertainty for package developers that want to name their package with a . or _ in it because the actual local save location would be different. I'd rather like dots to not be allowed at all than renaming them behind the scenes.

Pinging @wbond~

FichteFoll avatar Jan 06 '14 01:01 FichteFoll

I just spent a bunch of time trying to see if we can work around importing packages with a . in the name. It isn't really feasible. There are issues the using the imp module for imports related to non-ascii file paths. I know Jon spent a bunch of time dealing with that in the past.

I'm thinking this issue should be resolve by:

  1. Adding a note to the instructions to tell developers not to name their package with a . if it uses Python and they want it to work on ST3
  2. Mark any existing packages with Python that don't work in ST3 and ST2 only compatible

We can let the individual package maintainers decide if they want to deal with the renaming issue.

wbond avatar Feb 05 '14 02:02 wbond

  1. Yes, we should definitely do this.
  2. I'm not sure I understand that, but I think you meant to mark packages that include Python code and are marked as ST3-compatible to be ST2-only, correct?

FichteFoll avatar Feb 05 '14 14:02 FichteFoll

It might be best to just restrict the use of . all together. Someone might not remember that they cannot have a python file if their repo name has a . in it, and later include some python in the package.

joneshf avatar Feb 15 '14 00:02 joneshf

It might be best to just restrict the use of . all together.

There would certainly be a lot less room for confusion.

icio avatar Feb 15 '14 12:02 icio

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.

FichteFoll avatar Feb 15 '14 14:02 FichteFoll

If @wbond gives the okay. I'll do the leg work.

joneshf avatar Feb 15 '14 17:02 joneshf

@wbond what's the verdict?

joneshf avatar Mar 07 '14 17:03 joneshf

Since February we've had in the docs instructions not to use . in package names. See https://github.com/wbond/sublime.wbond.net/commit/1ab20e3c61fd1c5af85bde4fd43384042866a187 for reference.

We should notify all package maintainers with a . that they have a week to pick a new name, or we will mark the package only compatible with ST2. Then we can tweak the tests to not worry about the . test if the package is marked for ST2 only.

Here are the packages that advertise ST3 compatibility and have a . in the name:

Play 2.0 https://github.com/guillaumebort/play2-sublimetext2

objc .strings syntax language https://github.com/PaNaVTEC/Objc-Strings-Syntax-Language

Codepad.org Paste Plugin https://github.com/dangelov/Paste-Codepad.org

Dust.js https://github.com/sntran/sublime-dustjs

Animate.css Snippets https://github.com/adeniszczyc/Animate.css-Snippets

Backbone.js https://github.com/tomasztunik/Sublime-Text-2-Backbone.js-package

Theme - itg.flat https://github.com/itsthatguy/theme-itg-flat

Todo.txt Syntax https://github.com/dertuxmalwieder/SublimeTodoTxt

D3.js Snippets https://github.com/fabriciotav/d3-snippets-for-sublime-text-2

Kohana 2.x Snippets https://github.com/golf3gtiii/Kohana234-sublimeText2-plugin

Mou.app Markdown https://github.com/rwoody/mou-markdown-sublime

FakeImg.pl Image Placeholder Snippet https://github.com/tinacious/fakeimg.sublime-snippet

sublimetext.github.com https://github.com/SublimeText/sublimetext.github.com

ApacheConf.tmLanguage https://github.com/colinta/sublime_packages

Chaplin.js https://github.com/joneshf/sublime-chaplinjs

jQuery Mobile 1.4 Snippets https://github.com/MobPro/Sublime-jQuery-Mobile-Snippets

Three.js Autocomplete https://github.com/blackjk3/threejs-sublime

Lazy Backbone.js https://github.com/pwhisenhunt/Sublime-Text-2-Lazy-Backbone.js-Package

PEG.js https://github.com/alexstrat/PEGjs.tmbundle

Ember.js Snippets https://github.com/fabriciotav/ember-snippets-for-sublime-text-2

Backbone.Marionette https://github.com/amokan/ST2-Backbone.Marionette

Conky.tmLanguage https://github.com/oliverseal/Conky.tmLanguage

Require Node.js Modules Helper https://github.com/jfromaniello/sublime-node-require

Underscore.js Snippets https://github.com/carlo/sublime-underscorejs-snippets

wpseek.com WordPress Developer Assistant https://github.com/AlphawolfWMP/sublime-text-2-wpseek

wbond avatar Jul 06 '14 00:07 wbond

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.

FichteFoll avatar Jul 06 '14 00:07 FichteFoll

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 Sublime Text 2 supported only?

@FichteFoll, I reviewed every package on the list @wbond posted and opened a issue on each repository:

  1. Some repositories where moved and I followed their link change.
  2. Some repositories where not found on GitHub, but they still present on the channel.json
  3. Some repositories where already renamed removing the dot . on the name

Not found

  1. [ ] 1) Codepad.org Paste Plugin i. (Page not Found) Perhaps remove it from package control? ii. https://github.com/dangelov/Paste-Codepad.org

  2. [ ] 2) objc .strings syntax language i. (Page not Found) Perhaps remove it from package control? ii. https://github.com/PaNaVTEC/Objc-Strings-Syntax-Language

  3. [ ] 3) Mou.app Markdown i. (Page not Found) Perhaps remove it from package control? ii. Already renamed to Mou Markdown App (OSX) on 235c73a01712dab59f9f84c257df705cd3b1c1d6 iii. https://github.com/rwoody/mou-markdown-sublime

Removed from the channel

  1. [x] 1) sublimetext.github.com i. Removed from the channel on 4e8a465c38ac161c345408484f17d3cb262b01e6 ii. https://github.com/SublimeText/sublimetext.github.com

Already renamed

  1. [x] 1) Require Node.js Modules Helper i. Already renamed to Require CommonJS Modules Helper on 8a8659ce9d46ddc69abfe53bcc156954165c3985 ii. https://github.com/jfromaniello/sublime-node-require

  2. [x] 2) Dust.js i. Already renamed to DustBuster on 7b772d11bb33e951318adcb350c11a5e688eb842 ii. https://github.com/sntran/sublime-dustjs

  3. [x] 3) ApacheConf.tmLanguage i. Issue opened on the original repository asking to rename ii. https://github.com/colinta/ApacheConf.tmLanguage/issues/20 Remove the dot . on the package name ApacheConf.tmLanguage iii. Already renamed to ApacheConf on https://github.com/colinta/sublime_packages/commit/e41b0eb78ae755b218bc9051cc7cd5957f1708e1

Renaming pending

  1. [ ] 1) Play 2.0 i. Issue opened on the original repository asking to rename ii. https://github.com/guillaumebort/play2-sublimetext2/issues/32 Remove the dot . on the package name Play 2.0

  2. [ ] 2) Animate.css Snippets i. Issue opened on the original repository asking to rename ii. https://github.com/adeniszczyc/Animate.css-Snippets/issues/3 Remove the dot . on the package name Animate.css Snippets

  3. [x] 3) Backbone.js i. Issue opened on the original repository asking to rename ii. https://github.com/tomasztunik/Sublime-Text-2-Backbone.js-package/issues/4 Remove the dot . on the package name Backbone.js

  4. [ ] 4) Theme - itg.flat i. Issue opened on the original repository asking to rename ii. https://github.com/itsthatguy/theme-itg-flat/issues/91 Remove the dot . on the package name Theme - itg.flat

  5. [x] 5) Todo.txt Syntax i. Issue opened on the original repository asking to rename ii. https://github.com/dertuxmalwieder/SublimeTodoTxt/issues/23 Remove the dot . on the package name Todo.txt Syntax

  6. [ ] 6) D3.js Snippets i. Issue opened on the original repository asking to rename ii. https://github.com/fabriciotav/d3-snippets-for-sublime-text-2/issues/1 Remove the dot . on the package name D3.js Snippets

  7. [ ] 7) Kohana 2.x Snippets i. Issue opened on the original repository asking to rename ii. https://github.com/golf3gtiii/Kohana234-sublimeText2-plugin/issues/1 Remove the dot . on the package name Kohana 2.x Snippets

  8. [ ] 8) FakeImg.pl Image Placeholder Snippet i. Issue opened on the original repository asking to rename ii. https://github.com/tinacious/fakeimg.sublime-snippet/issues/2 Remove the dot . on the package name FakeImg.pl Image Placeholder Snippet

  9. [ ] 9) Chaplin.js i. Issue opened on the original repository asking to rename ii. https://github.com/joneshf/sublime-chaplinjs/issues/1 Remove the dot . on the package name Chaplin.js

  10. [x] 10) jQuery Mobile 1.4 Snippets i. Issue opened on the original repository asking to rename ii. https://github.com/MobPro/Sublime-jQuery-Mobile-Snippets/issues/1 Remove the dot . on the package name jQuery Mobile 1.4 Snippets

  11. [ ] 11) Three.js Autocomplete i. Issue opened on the original repository asking to rename ii. https://github.com/blackjk3/threejs-sublime/issues/4 Remove the dot . on the package name Three.js Autocomplete

  12. [ ] 12) Lazy Backbone.js i. Issue opened on the original repository asking to rename ii. https://github.com/pwhisenhunt/Sublime-Text-2-Lazy-Backbone.js-Package/issues/1 Remove the dot . on the package name Lazy Backbone.js

  13. [ ] 13) PEG.js i. Issue opened on the original repository asking to rename ii. https://github.com/alexstrat/PEGjs.tmbundle/issues/3 Remove the dot . on the package name PEG.js

  14. [ ] 14) Ember.js Snippets i. Issue opened on the original repository asking to rename ii. https://github.com/fabriciotav/ember-snippets-for-sublime-text-2/issues/4 Remove the dot . on the package name Ember.js Snippets

  15. [ ] 15) Backbone.Marionette i. Issue opened on the original repository asking to rename ii. https://github.com/amokan/ST2-Backbone.Marionette/issues/3 Remove the dot . on the package name Backbone.Marionette

  16. [ ] 16) Conky.tmLanguage i. Issue opened on the original repository asking to rename ii. https://github.com/oliverseal/Conky.tmLanguage/issues/4 Remove the dot . on the package name Conky.tmLanguage

  17. [ ] 17) Underscore.js Snippets i. Issue opened on the original repository asking to rename ii. https://github.com/carlo/sublime-underscorejs-snippets/issues/8 Remove the dot . on the package name Underscore.js Snippets

  18. [x] 18) wpseek.com WordPress Developer Assistant i. Issue opened on the original repository asking to rename ii. https://github.com/wpseek/sublime-text-2-wpseek/issues/6 Remove the dot . on the package name wpseek.com WordPress Developer Assistant

evandrocoan avatar Jan 04 '18 08:01 evandrocoan

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.

FichteFoll avatar Jan 09 '18 16:01 FichteFoll

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

Acecool avatar Oct 16 '18 12:10 Acecool

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.

evandrocoan avatar Oct 16 '18 19:10 evandrocoan

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.

FichteFoll avatar Oct 30 '18 20:10 FichteFoll

Could this be given more visibility ( documentation update would be great ), to give a heads-up to future package writer ?

dvhh avatar Mar 12 '19 10:03 dvhh

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.

braver avatar Jan 04 '23 10:01 braver