Textmate conversion error reporting & apm list --enabled #729
Conversation
FrederickGeek8
changed the title
FrederickGeek8
changed the title
FrederickGeek8
changed the title
FrederickGeek8
changed the title
src/list.coffee
Outdated
| @@ -81,11 +83,11 @@ class List extends Command | |||
| manifest ?= {} | |||
| manifest.name = child | |||
| if options.argv.themes | |||
| packages.push(manifest) if manifest.theme | |||
| packages.push(manifest) if manifest.theme and not (options.argv.enabled and @isPackageDisabled(child)) | |||
There was a problem hiding this comment.
I think these inline ifs are getting a bit hard to understand at a glance. Would you mind converting them into regular ifs? Eg
if mainfest.theme and not (options.argv.enabled and @isPackageDisabled(manifest.name))Also, manifest.name is clearer in its intent than just child.
src/package-converter.coffee
Outdated
| selector = new ScopeSelector(scope).toCssSelector() if scope | ||
| catch e | ||
| e.fileName = path.join(sourceSnippets, child) | ||
| e.message = "In file " + e.fileName + " at " + JSON.stringify(scope) + ": " + e.message |
There was a problem hiding this comment.
What do you think about using interpolation here instead?
src/package-converter.coffee
Outdated
| try | ||
| selector = new ScopeSelector(scope).toCssSelector() if scope | ||
| catch e | ||
| e.fileName = path.join(sourceSnippets, child) |
There was a problem hiding this comment.
The fileName property seems to be non-standard and only works in Firefox. I'm not sure we need this considering the filename is printed anyway in the message.
|
@50Wliu I changed what you requested and added some Jasmine tests. |
spec/list-spec.coffee
Outdated
| @@ -88,6 +88,17 @@ describe 'apm list', -> | |||
| listPackages [], -> | |||
| expect(console.log.argsForCall[1][0]).toContain '[email protected] (disabled)' | |||
|
|
|||
| it 'displays only enabled packages --enabled is called', -> | |||
There was a problem hiding this comment.
Think you forgot a when here :)
Also, can you also copy over a different (enabled) package just to confirm that the output isn't always (empty)? 😀
There was a problem hiding this comment.
Sorry about that... Should be fixed now as well as a layer of verification that the package field is not always empty.
|
@50Wliu just a heads up, I made the requested changes |
|
Thanks! |
|
Hi, --enabled was already available in #623 Curious why that one was never reviewed or merged. |
Description of the Change
Change 1: Textmate Bundle Conversion Error Reporting
My change is in response to atom/first-mate#51 – an issue which requested that Atom Package Manager give more information, such as file names and line numbers, in regards to errors when converting TextMate Bundle files to an Atom digestible format.
I decided to apply the fix in apm instead of first-mate since the information required to locate the source of the error is not made available to first-mate. Additionally, it appears to be near impossible (or at least efficient) to ascertain an exact location of an error, so I elected to output the malformed piece of text instead, which should hopefully give enough information to the user to debug their program.
Change 2: New
apm listcommandDue to a mistake on my part, instead of opening a new pull request for this change, I accidentally merged it into this pull request. The second change is one that fulfills #732 – an issue that requests a feature that allowed for the command
apm list --installed --enabledto list enabled packages in Atom.Alternate Designs
My first thought was to feed the scope variable (that had a chance to be malformed, causing the crash), back into a regex on the original file, allowing the error position to be ascertained. However, given how the Package Converter is structured, it would be too computationally intensive to perform such an action.
No alternate design was considered for the second change. I do not see any method as simple and concise as the approach I took
Benefits
Better error reporting during TextMate Bundle conversion.
Possible Drawbacks
I see no possible drawbacks to this change.
Applicable Issues
atom/first-mate#51