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

Test against package names containing periods #2524

Closed
wants to merge 1 commit into from

Conversation

icio
Copy link
Contributor

@icio icio commented Dec 22, 2013

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:

$ 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'

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.
@FichteFoll
Copy link
Collaborator

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.

@icio
Copy link
Contributor Author

icio commented Dec 23, 2013

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.

@joneshf
Copy link
Collaborator

joneshf commented Jan 6, 2014

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.

@FichteFoll
Copy link
Collaborator

  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~

@wbond
Copy link
Owner

wbond commented Feb 5, 2014

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.

@FichteFoll
Copy link
Collaborator

  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?

@joneshf
Copy link
Collaborator

joneshf commented Feb 15, 2014

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.

@icio
Copy link
Contributor Author

icio commented Feb 15, 2014

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

There would certainly be a lot less room for confusion.

@FichteFoll
Copy link
Collaborator

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.

@joneshf
Copy link
Collaborator

joneshf commented Feb 15, 2014

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

@joneshf
Copy link
Collaborator

joneshf commented Mar 7, 2014

@wbond what's the verdict?

@wbond
Copy link
Owner

wbond commented Jul 6, 2014

Since February we've had in the docs instructions not to use . in package names. See wbond/packagecontrol.io@1ab20e3 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

@FichteFoll
Copy link
Collaborator

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.

@evandrocoan
Copy link
Contributor

evandrocoan commented Jan 4, 2018

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 235c73a
    iii. https://github.com/rwoody/mou-markdown-sublime

Removed from the channel

  1. 1) sublimetext.github.com
    i. Removed from the channel on 4e8a465
    ii. https://github.com/SublimeText/sublimetext.github.com

Already renamed

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

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

  3. 3) ApacheConf.tmLanguage
    i. Issue opened on the original repository asking to rename
    ii. Remove the dot . on the package name ApacheConf.tmLanguage colinta/ApacheConf.tmLanguage#20 Remove the dot . on the package name ApacheConf.tmLanguage
    iii. Already renamed to ApacheConf on colinta/sublime_packages@e41b0eb

Renaming pending

  1. 1) Play 2.0
    i. Issue opened on the original repository asking to rename
    ii. Remove the dot . on the package name Play 2.0 guillaumebort/play2-sublimetext2#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. Remove the dot . on the package name Animate.css Snippets adeniszczyc/Animate.css-Snippets#3 Remove the dot . on the package name Animate.css Snippets

  3. 3) Backbone.js
    i. Issue opened on the original repository asking to rename
    ii. Remove the dot . on the package name Backbone.js tomasztunik/Sublime-Text-2-Backbone.js-package#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. Remove the dot . on the package name Theme - itg.flat itsthatguy/theme-itg-flat#91 Remove the dot . on the package name Theme - itg.flat

  5. 5) Todo.txt Syntax
    i. Issue opened on the original repository asking to rename
    ii. Remove the dot . on the package name Todo.txt Syntax dertuxmalwieder/SublimeTodoTxt#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. Remove the dot . on the package name D3.js Snippets fabriciotav/d3-snippets-for-sublime-text-2#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. Remove the dot . on the package name Kohana 2.x Snippets golf3gtiii/Kohana234-sublimeText2-plugin#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. Remove the dot . on the package name FakeImg.pl Image Placeholder Snippet tinacious/fakeimg.sublime-snippet#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. Remove the dot . on the package name Chaplin.js joneshf/sublime-chaplinjs#1 Remove the dot . on the package name Chaplin.js

  10. 10) jQuery Mobile 1.4 Snippets
    i. Issue opened on the original repository asking to rename
    ii. Remove the dot . on the package name jQuery Mobile 1.4 Snippets Sage-Archer/sublime-jquery-mobile-snippets#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. Remove the dot . on the package name Three.js Autocomplete blackjk3/threejs-sublime#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. Remove the dot . on the package name Lazy Backbone.js pwhisenhunt/Sublime-Text-2-Lazy-Backbone.js-Package#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. Remove the dot . on the package name PEG.js alexstrat/PEGjs.tmbundle#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. Remove the dot . on the package name Ember.js Snippets fabriciotav/ember-snippets-for-sublime-text-2#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. Remove the dot . on the package name Backbone.Marionette amokan/ST2-Backbone.Marionette#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. Remove the dot . on the package name Conky.tmLanguage oliverseal/Conky.tmLanguage#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. Remove the dot . on the package name Underscore.js Snippets czottmann/sublime-underscorejs-snippets#8 Remove the dot . on the package name Underscore.js Snippets

  18. 18) wpseek.com WordPress Developer Assistant
    i. Issue opened on the original repository asking to rename
    ii. Remove the dot . on the package name wpseek.com WordPress Developer Assistant wpseek/sublime-text-2-wpseek#6 Remove the dot . on the package name wpseek.com WordPress Developer Assistant

emetselaar added a commit to emetselaar/package_control_channel that referenced this pull request Jan 4, 2018
Updated name of jQuery Mobile 14 Snippets to not include a dot

As suggested in wbond#2524
oliverschloebe added a commit to wpseek/package_control_channel that referenced this pull request Jan 4, 2018
@FichteFoll
Copy link
Collaborator

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.

@Acecool
Copy link

Acecool commented Oct 16, 2018

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

@evandrocoan
Copy link
Contributor

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.

@FichteFoll
Copy link
Collaborator

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.

@dvhh
Copy link
Contributor

dvhh commented Mar 12, 2019

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

@braver
Copy link
Collaborator

braver commented Jan 4, 2023

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 braver closed this Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants