Skip to content

Treat default_view as view by default#6758

Closed
andrey-git wants to merge 1 commit into
devfrom
andrey-git-patch-1
Closed

Treat default_view as view by default#6758
andrey-git wants to merge 1 commit into
devfrom
andrey-git-patch-1

Conversation

@andrey-git
Copy link
Copy Markdown
Contributor

@andrey-git andrey-git commented Mar 23, 2017

Description:

Treat default_view as view by default
Redo of home-assistant/home-assistant-js-websocket/pull/6

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@mention-bot
Copy link
Copy Markdown

@andrey-git, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @pvizeli and @fabaff to be potential reviewers.

@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 24, 2017

I don't think that this is the right solution either. We should raise an error or else people might be confused on how to specify a view because there are two ways.

We should also test this.

@andrey-git
Copy link
Copy Markdown
Contributor Author

I will add tests, but requiring view: true for default_view would be a breaking change. What is the benefit of forcing a more verbose syntax on the user?

@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 24, 2017

Both approaches will be breaking changes. Because someone might want to have a group called default_view and use it as a group (I doubt it though).

I don't like auto-fixing config because it is a slippery slope. We start auto fixing more and more and all of a sudden we get into a situation where 2 auto-fixes collide and we have to start making a decision that can break things for some users… so better to never auto fix.

@andrey-git
Copy link
Copy Markdown
Contributor Author

So maybe make a fix in a different direction - make default_view without view: true behave like a regular non-view group? This way it would be consistent, and default_view wouldn't be a magic name in the backend, just in the frontend.

@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 28, 2017

Isn't it already treated as a non-view group ?

@andrey-git
Copy link
Copy Markdown
Contributor Author

@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 29, 2017

Good point! Yes, we should update those pieces of code.

@andrey-git
Copy link
Copy Markdown
Contributor Author

Created home-assistant/frontend#251

@balloob balloob closed this Apr 3, 2017
@balloob balloob deleted the andrey-git-patch-1 branch April 3, 2017 00:46
@home-assistant home-assistant locked and limited conversation to collaborators Jul 17, 2017
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.

5 participants