search

atom / snippets

Atom snippets package
MIT License
205 stars 100 forks source link

Added more formatting options for snippets #194

Closed PierBover closed 8 years ago

PierBover commented 8 years ago

When working on a snippet I was wondering how other packages formatted the results in the autocomplete list. So I found the autocomplete-plus API can accept more parameters for formatting the results.

leftLabel (optional): This is shown before the suggestion. Useful for return values. leftLabelHTML (optional): Use this instead of leftLabel if you want to use html for the left label. rightLabel (optional): An indicator (e.g. function, variable) denoting the "kind" of suggestion this represents rightLabelHTML (optional): Use this instead of rightLabel if you want to use html for the right label.

I thought it would be awesome to be able to visually format results for snippets too, so I have added those properties in the Snippet class so that you could add those in the cson files that define snippets.

The idea is to read those optional properties from the cson file and provide them to autocomplete-snippets which in turn will deliver those results to autocomplete-plus.

Additions to the Snippet class (All parameters are optional)

I haven't added rightLabel because autocomplete-snippets uses the name of the snippet to fill that and is what all current snippets use to display some info.

I have also worked on autocomplete-snippets locally, but I will wait for this pull request to be accepted before submitting it there.

lee-dohm commented 8 years ago

To be accepted, this extra functionality would need specs and updates to the documentation in the README.

PierBover commented 8 years ago

Thanks for the suggestions @lee-dohm, I've added a description of the fork in my first comment and updated the readme.

PierBover commented 8 years ago

Is there anything else I should do?

50Wliu commented 8 years ago

@PierBover The new functionality still needs specs to make sure it doesn't regress in the future.

PierBover commented 8 years ago

Thanks @50Wliu

Could you point me to an example of the type of specs you are expecting?

50Wliu commented 8 years ago

Hmm, it looks like there aren't any tests for the existing parameters either :frowning:. Since all you're doing here is passing some new params along and not actually doing anything with them, I think it'd actually be fine not to add specs here and instead add them in autocomplete-snippets or autocomplete-plus. Thoughts @lee-dohm?

PierBover commented 8 years ago

I've also modified autocomplete-snippets to receive those parameters and pass them to autocomplete-plus. Should I add a PR there or wait for this PR to be implemented here first?

lee-dohm commented 8 years ago

I'd expect to at least see something like this that shows the new snippet values get parsed correctly from a sample snippet file:

https://github.com/atom/snippets/blob/master/spec/snippet-loading-spec.coffee#L33-L49

PierBover commented 8 years ago

@lee-dohm it seems the part you highlighted is for required arguments, although since this is the first time I'm writing specs for Atom and writing in CS I'm not so sure.

I've updated test.cson with 2 more cases using these new optional properties, and snippet-loading-spec.coffee too to test for that (I think).

Checks have passed... let me know if there's anything else I must do.

PierBover commented 8 years ago

Hey @lee-dohm and @50Wliu

The modifications I made to the snippets class and specs are identical (obviously with different parameters) to these ones added last year.

https://github.com/atom/snippets/pull/132

And that PR was accepted...

Is there any reason why you won't accept my PR? What else do you want me to do?

lee-dohm commented 8 years ago

Thanks for the ping @PierBover. I missed your earlier post. Unfortunately with a project this size, sometimes that happens :disappointed:

On the face of it, this looks good. I need to do some more testing and then if everything is working, this should be good to go.

PierBover commented 8 years ago

Awesome!

@lee-dohm are you also in charge of autocomplete-snippets ?

lee-dohm commented 8 years ago

I don't know that I'm necessarily "in charge" of anything :grinning: But I've got my fingers in everything :laughing: Why do you ask?

PierBover commented 8 years ago

:)

Sorry... I'm a bit of a noob at contributing to Atom.

I was asking because if / when this PR gets accepted I will need to make a PR in autocomplete-snippets to be able to get the full functionality. I don't know if I should wait to do that or not.

lee-dohm commented 8 years ago

You can create it now and just say in the PR comment that it is dependent on this one.

On Fri, Mar 18, 2016 at 7:13 PM, Pier Bover notifications@github.com wrote:

:)

Sorry... I'm a bit of a noob at contributing to Atom.

I was asking because if / when this PR gets accepted I will need to make a PR in autocomplete-snippets to be able to get the full functionality. I don't know if I should wait to do that or not.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/atom/snippets/pull/194#issuecomment-198615707

lee-dohm commented 8 years ago

Thanks for contributing! Sorry for the delay :grinning:

PierBover commented 8 years ago

Yay! :)

I've been super busy but I will create the PR on autocomplete-snippets these days.