Skip to content

WidgetTest: Check types of settings#35

Closed
VesnaT wants to merge 1 commit intobiolab:masterfrom
VesnaT:setting_types
Closed

WidgetTest: Check types of settings#35
VesnaT wants to merge 1 commit intobiolab:masterfrom
VesnaT:setting_types

Conversation

@VesnaT
Copy link
Contributor

@VesnaT VesnaT commented Jan 7, 2020

Issue

Fixes #20

Description of changes

Add settings checking into tearDown of WidgetTest.
The change causes the following tests (in biolab/orange3) to fail:

Includes
  • Code changes
  • Tests
  • Documentation

@VesnaT VesnaT added the needs discussion Core developers need to discuss the issue label Jan 7, 2020
@janezd
Copy link
Contributor

janezd commented Jan 9, 2020

What if this PR showed warnings instead of raising errors for now? It would warn that after 2/1/21 they would change into errors, though.

In its current form, we shouldn't merge it. :)

@janezd
Copy link
Contributor

janezd commented Jan 12, 2020

Why is OWFile not on this list although it stores a list of RecentPath?

@ales-erjavec, did I understand correctly that the idea is to avoid pickling settings?

Context settings are stored as instances of ContextSetting. OWFile stores RecentPath instances. OWColor stores (essentially) namedtuples with some properties... Adapting this would burden the widgets with code that would essentially perform serialization of settings. I wonder whether the benefits of having json outweigh the trouble and the extra code for conversion (with its maintenance and potential bugs).

Or are there other reasons for limiting the allowed types for settings?

@ales-erjavec
Copy link
Collaborator

did I understand correctly that the idea is to avoid pickling settings?

Yes.

@VesnaT
Copy link
Contributor Author

VesnaT commented Jan 14, 2020

Why is OWFile not on this list although it stores a list of RecentPath?

Because TestOWFile overrides tearDown().

@janezd
Copy link
Contributor

janezd commented Mar 12, 2021

Superseeded by #62.

@janezd janezd closed this Mar 12, 2021
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.

Check types of settings

4 participants