Skip to content

Conversation

kelunik
Copy link
Member

@kelunik kelunik commented Jan 8, 2017

This ensures it's clear which info is returned. I could have added another sentence to the docblock instead, but I think this makes it more obvious when consuming the code, too.

In general, I think that we should be careful with allowing extension here, as it limits us in adding more specified keys, such as "disabled_watchers".

This ensures it's clear which info is returned.
@kelunik
Copy link
Member Author

kelunik commented Jan 8, 2017

This is a PR for #131.

Copy link
Member

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge tomorrow if no-one else objects.

@kelunik
Copy link
Member Author

kelunik commented Jan 9, 2017

@bwoebi: What do you think about limiting extension to allow for future extension in the specification?

@bwoebi
Copy link
Member

bwoebi commented Jan 9, 2017

Once we merge the separate virtual package for API, this isn't an issue.
Yes, it will require a major version for the Driver API, but not for the loop implementations themselves. (unless the implementations define a key with the same name but different value; in that case semver following implementations have to release a major version - but that's then specific for the applications implementing that particular loop.)

I do not see that future extension is exceedingly limited here.

@kelunik
Copy link
Member Author

kelunik commented Jan 9, 2017

But those implementations have to release a new major version. IMO they should add a separate method if they want to add more data, that will have a way lower chance of conflicting. Additionally, this would make the return of getInfo completely predictable.

@bwoebi
Copy link
Member

bwoebi commented Jan 9, 2017

Well, it's the choice of the implementations whether they choose to add more entries and incur the risk of having to sometime release a new major in the unlikely case of same key, different value.

The same way round you may argue that you could - without this restriction - rename keys in the Driver without requiring the implementations to release a new major (because they're allowed to retain the old key then).

@kelunik
Copy link
Member Author

kelunik commented Jan 9, 2017

The same way round you may argue that you could - without this restriction - rename keys in the Driver without requiring the implementations to release a new major (because they're allowed to retain the old key then).

No, you can't. This would break the API. I'm specifically talking about not breaking the consumer API, but still being able to break the SPI.

@kelunik
Copy link
Member Author

kelunik commented Jan 10, 2017

@trowski @rdlowrey @WyriHaximus How do you think about the extension?

Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes perfect sense to me 👍

@kelunik
Copy link
Member Author

kelunik commented Jan 13, 2017

Could anyone merge this?

@bwoebi bwoebi merged commit a1e0ebb into master Jan 13, 2017
@kelunik kelunik deleted the count-watchers branch January 13, 2017 12:38
@bwoebi bwoebi mentioned this pull request Jan 15, 2017
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.

3 participants