Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Get priority from config #98

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

elliotaplant
Copy link

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

This change allows users to configure the inclusionPriority and suggestionPriority of their snippets in the autocomplete menu

Alternate Designs

An alternative considered was to increase the default priorities of this package, but that just contributes to a priority arms race.

Benefits

The benefit of this package is that users will be able to control the priority of their snippets without changing the behavior for current users.

Possible Drawbacks

One possible drawback is that users may configure their priorities incorrectly and not know why their snippets are prioritized too hight or too low.

Applicable Issues

atom/autocomplete-plus#946
atom/snippets#265
#86
#94

@samartioli
Copy link

awesome!

@elliotaplant elliotaplant changed the title Ep get priority from config Get priority from config Oct 28, 2018
@rsese
Copy link

rsese commented Nov 1, 2018

Thanks @elliotaplant!

Can you add tests? As mentioned in the Requirements section, we require tests to guard against regressions so tests are necessary before the team can review.

@elliotaplant
Copy link
Author

Of course. I'll add them this week

@elliotaplant
Copy link
Author

@rsese - I added a couple tests for the default case and the change behavior. Let me know if those work for you!

@rsese
Copy link

rsese commented Nov 5, 2018

Thank you! Someone from the team will take a look as soon as they can.

@elliotaplant
Copy link
Author

No rush @rsese, but has anyone had a chance to take a peek at this?

@Aerijo
Copy link
Contributor

Aerijo commented Dec 12, 2018

Not sure if I count as team, but see the requested changes.

@elliotaplant Have you also confirmed this works in practice? I've had trouble before with autocomplete-plus not picking up on the provider changing settings.

Edit: It looks like it should work though, because it's grabbing the property from the provider object each time. I was having trouble with blacklisting selectors which are just copied in the beginning.

@elliotaplant
Copy link
Author

Thanks for the suggestions @Aerijo! I'll make the changes when I get a chance.

@rsese
Copy link

rsese commented Dec 12, 2018

Not sure if I count as team, but see the requested changes.

You definitely count as part of the team and I know your reviews are appreciated 😄

As far as reviewing to the point of merging or not however, one of the GitHub maintainers would do that - so to circle back to your question about timing @elliotaplant, with the team's finite resources and current workload, we can't promise a particular timeframe when this would be reviewed at the moment.

But it's definitely on the team's radar so when someone is able to take a look, they'll comment here.

@rsese
Copy link

rsese commented Dec 12, 2018

But it's definitely on the team's radar so when someone is able to take a look, they'll comment here.

Err sorry, to clarify I mean once @Aerijo's requested changes are addressed please comment back here and we'll ask the team to take a look again when they're able to.

@TiriaAndersen
Copy link

@elliotaplant Is this still progressing? Or has it been ostensibly dropped?

@adekbadek
Copy link

adekbadek commented Apr 20, 2022

Hi @elliotaplant – this would be a really cool feature, any chance you might have a look at the requested changes?

@elliotaplant
Copy link
Author

Sorry for the multi-year delay! I addressed @Aerijo's requested changes, so hopefully @rsese or someone else on the team can take a look. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants