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

Fix non-syncthru supporting printers #21482

Open
wants to merge 15 commits into
base: dev
from

Conversation

@nielstron
Copy link
Contributor

nielstron commented Feb 27, 2019

Description:

Until now, the component would crash if syncthru was not supported by the printer. This is fixed now.

Also the component now uses the async web communication provided by the newer pysyncthru lib.

Related issue (if applicable): fixes #19335

Pull request in home-assistant.io with documentation (if applicable): no

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

nielstron added some commits Feb 27, 2019

Show resolved Hide resolved homeassistant/components/sensor/syncthru.py Outdated
@@ -60,32 +61,36 @@
})


def setup_platform(hass, config, add_entities, discovery_info=None):
def async_setup_platform(hass, config, async_add_entities, discovery_info=None):

This comment has been minimized.

@houndci-bot

houndci-bot Mar 3, 2019

line too long (80 > 79 characters)

@nielstron nielstron force-pushed the nielstron:pysyncthru-patch branch from 909eb70 to 71872c0 Mar 3, 2019

Show resolved Hide resolved homeassistant/components/sensor/syncthru.py Outdated
Show resolved Hide resolved requirements_all.txt Outdated
await printer.update()
except ValueError:
# if an exception is thrown, printer does not support syncthru
_LOGGER.info("Samsung printer at %s does not support SyncThru", host)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Mar 16, 2019

Member

Shouldn't this be a warning?

This comment has been minimized.

@nielstron

nielstron Mar 23, 2019

Author Contributor

No, the printer can be discovered automatically but still may not support syncthru, then no warnings or errors should be thrown. Still the way I coded it here was incorrect, see new source code for correct handling.

Show resolved Hide resolved homeassistant/components/sensor/syncthru.py Outdated
# If the printer was discovered automatically, no warning or error
# should be issued and printer should not be set up
if discovery_info is not None:
_LOGGER.info("Samsung printer at %s does not support SyncThru", host)

This comment has been minimized.

@houndci-bot

houndci-bot Mar 21, 2019

line too long (81 > 79 characters)

DEFAULT_MONITORED_CONDITIONS.extend(['toner_{}'.format(key) for key in TONER_COLORS])
DEFAULT_MONITORED_CONDITIONS.extend(['drum_{}'.format(key) for key in DRUM_COLORS])
DEFAULT_MONITORED_CONDITIONS.extend(['trays_{}'.format(key) for key in TRAYS])
DEFAULT_MONITORED_CONDITIONS.extend(['output_trays_{}'.format(key) for key in OUTPUT_TRAYS])

This comment has been minimized.

@houndci-bot

houndci-bot Mar 21, 2019

line too long (92 > 79 characters)

OUTPUT_TRAYS = range(0, 6)
DEFAULT_MONITORED_CONDITIONS = []
DEFAULT_MONITORED_CONDITIONS.extend(['toner_{}'.format(key) for key in TONER_COLORS])
DEFAULT_MONITORED_CONDITIONS.extend(['drum_{}'.format(key) for key in DRUM_COLORS])

This comment has been minimized.

@houndci-bot

houndci-bot Mar 21, 2019

line too long (83 > 79 characters)

TRAYS = range(1, 6)
OUTPUT_TRAYS = range(0, 6)
DEFAULT_MONITORED_CONDITIONS = []
DEFAULT_MONITORED_CONDITIONS.extend(['toner_{}'.format(key) for key in TONER_COLORS])

This comment has been minimized.

@houndci-bot

houndci-bot Mar 21, 2019

line too long (85 > 79 characters)

nielstron added some commits Mar 21, 2019

@nielstron nielstron changed the title Fix non syncthru-syncthru supporting printers Fix non-syncthru supporting printers Mar 23, 2019


PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Required(CONF_RESOURCE): cv.url,
vol.Required(CONF_RESOURCE):
cv.url,

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Mar 24, 2019

Member

Please revert all pure formatting changes. We shouldn't mix that into a fix PR.

await self.syncthru.update()
except ValueError:
# if an exception is thrown, printer does not support syncthru
_LOGGER.info("Samsung printer at %s does not support SyncThru",

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Mar 24, 2019

Member

Won't this spam the log every 30 seconds update is called for non supported printers? That's not a good experience.

It would be better to not add those printers at all. Can't we check this also for the manually configured printers and fail set up if support is lacking?

Maybe fail set up if printer is off?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Mar 24, 2019

Member

Could we remove the entity if this error happens and ask the user to change the config?

There's no point in keeping the entity if this error happens, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.