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

Сhanged priority to use external converters #1821

Merged
merged 6 commits into from
Nov 30, 2020
Merged

Сhanged priority to use external converters #1821

merged 6 commits into from
Nov 30, 2020

Conversation

xyzroe
Copy link
Contributor

@xyzroe xyzroe commented Nov 29, 2020

findByZigbeeModel and findByDevice return last candidate of all possible

Please check is it possible to do like this?
I got everything working.

findByZigbeeModel and findByDevice return last candidate of all possible
Trailing spaces removed
@Koenkk
Copy link
Owner

Koenkk commented Nov 29, 2020

Instead of doing this I prefer to add an additional parameter to

function addDefinition(definition) {
called addInFront (default false) and set that to true here: https://github.com/Koenkk/zigbee2mqtt/blob/master/lib/extension/externalConverters.js#L13

@xyzroe
Copy link
Contributor Author

xyzroe commented Nov 29, 2020

And then, at

function findByZigbeeModel(zigbeeModel) {
and
function findByDevice(device) {
check if exists any item with addInFront = true, return than one? Right?
Ok I'l work on it later.

Add an additional parameter to addDefinition called addInFront (default false)
Add an additional parameter to addDefinition called addInFront (default false)
Add an additional parameter to addDefinition called addInFront (default false)
@xyzroe
Copy link
Contributor Author

xyzroe commented Nov 29, 2020

I tried, but I'm not so good at tests writing.
But I checked everything manually.
I am my opinion everything works as expected.

@nurikk
Copy link
Contributor

nurikk commented Nov 30, 2020

Instead of doing this I prefer to add an additional parameter to

function addDefinition(definition) {

called addInFront (default false) and set that to true here: https://github.com/Koenkk/zigbee2mqtt/blob/master/lib/extension/externalConverters.js#L13

Why do we need to do this? Why not let to override internal converter by default? This behaviour is quite safe, since user need to do two steps:

  1. put external converter in data folder
  2. add external converter into configuration.json
    This can't cause any unexpected behaviour.

If we implement loading in this way, then we just need to pick last registered converter:

return candidates ? candidates[0] : null;

- return candidates ? candidates[0] : null; 
+ return candidates ? candidates[candidates.length - 1] : null;

@xyzroe
Copy link
Contributor Author

xyzroe commented Nov 30, 2020

I started from made returning last candidate of all possible, it is the easiest way to implement this function.

@Koenkk
Copy link
Owner

Koenkk commented Nov 30, 2020

@xyzroe sorry to switch opinions, but I think the solution provided by @nurikk is better indeed.

So the only changed required here is:

- return candidates ? candidates[0] : null; 
+ return candidates ? candidates[candidates.length - 1] : null;

@xyzroe
Copy link
Contributor Author

xyzroe commented Nov 30, 2020

How about findByDevice() ?
I think it is also necessary to change the order

@nurikk
Copy link
Contributor

nurikk commented Nov 30, 2020

How about findByDevice() ?
I think it is also necessary to change the order

Yes, apply this logic everywhere where it necessary. I just gave small example

findByZigbeeModel and findByDevice return last candidate of all possible
@xyzroe
Copy link
Contributor Author

xyzroe commented Nov 30, 2020

returned everything as it was originally in the commit e064dd9

@Koenkk
Copy link
Owner

Koenkk commented Nov 30, 2020

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants