Code Monkey home page Code Monkey logo

Comments (28)

freget avatar freget commented on July 28, 2024

It works with

database:
  class_name: Database
  class_path: plugins.database
  driver: sqlite3
  connect:
    - "database: /home/smarthome/log.db"
    - "check_same_thread: 0"

We should either change the documentation or (probably better) allow proper yaml parameters.

from plugins.

msinn avatar msinn commented on July 28, 2024

It works if your type it like it is written in the documentation (without spaces after the colons in the option stings)

Those Option Strings are not YAML and have to be formed the way the chosen database (mysql, sqlite3 or others) system describes them.

from plugins.

freget avatar freget commented on July 28, 2024

I really dislike code that is space sensitive.

My suggestion would be to change the documentation to the version with explicit quotes. This is much clearer to read and is not affected by a missing space or not. Otherwise, at least give the hint that the strings are space sensitive.

from plugins.

ohinckel avatar ohinckel commented on July 28, 2024

I just updated the code (lib/db.py to support OrderedDict and plugins/database/__init.py__) to support the standard YAML configuration (documentation also update) in develop branch (of smarthome and plugins).

With the new version you're now able to configure the parameters (the old way, like the way mentioned above, and the new way) using YAML style:

database:
  ...
  connect:
    database: /home/smarthome/log.db
    check_same_thread: 0
```

from plugins.

freget avatar freget commented on July 28, 2024

Cool! Thank you!

from plugins.

psilo909 avatar psilo909 commented on July 28, 2024

my config is now crashing: 2020-01-05 11:30:51 ERROR lib.metadata plugin 'database': Invalid definition in metadata file 'plugins/database/plugin.yaml': type 'dict(str)' for parameter 'connect' -> u
sing type 'foo' instead

database:
    plugin_name: database
    driver: pymysql
    connect:
    -   host:127.0.0.1
    -   user:root
    -   passwd:xxx
    -   port:3307
    -   db:smarthome
    instance: mysqldb

from plugins.

psilo909 avatar psilo909 commented on July 28, 2024

now tried to convert the config, which made it only worse:

2020-01-05 11:43:26 ERROR lib.metadata plugin 'database': Invalid definition in metadata file 'plugins/database/plugin.yaml': type 'dict(str)' for parameter 'connect' -> u
sing type 'foo' instead
2020-01-05 11:43:26 ERROR root Database [Database]: Could not connect to the database: %d format: a number is required, not str
2020-01-05 11:43:26 ERROR plugins.database mysqldb@: Database: Initialization failed: %d format: a number is required, not str

database:
    plugin_name: database
    driver: pymysql
    connect:
        host: 127.0.0.1
        user: root
        passwd: xxx
        port: 3307
        db: smarthome
    instance: mysqldb

from plugins.

bmxp avatar bmxp commented on July 28, 2024

@ohinckel Maybe you should still accept the old fashion parameters and give a deprecated warning when they are used. Otherwise this could be a breaking change for a lot of users that can't cope with it and it might well increase the requests for support in knxuf significantly...

from plugins.

psilo909 avatar psilo909 commented on July 28, 2024

i just switched the "dict(str)" to "dict" in plugin.yaml, as the documentation does not tell about dict() https://www.smarthomeng.de/developer/metadata/parameters.html. but now i am getting connection errors to localhost, which was working before?

from plugins.

psilo909 avatar psilo909 commented on July 28, 2024

i think one problem is, that the Port needs to be a number and not a str. so dict(str) is wrong:

ERROR root Database [Database]: Could not connect to the database: %d format: a number is required, not str <-- tried to specify it with port: '3307'

from plugins.

ohinckel avatar ohinckel commented on July 28, 2024

I change the plugin to keep the old parameter connect as is and introduced a new one named database. When using the old parameter a warning is logged to let the user know to update the configuration.

The new parameter database is not working currently, since I want to use dict as parameter type. But the metadata library issues an error since the test on the correct type only checks for a dict and not (additionally) for an OrderedDict which is created by the YAML parse - I guess.

Is there something what I'm doing wrong here? Are there other examples using dict as parameter type?

from plugins.

psilo909 avatar psilo909 commented on July 28, 2024

I change the plugin to keep the old parameter connect as is

so why did you change to to a dict in plugin.yaml then?

connect:
        type: list(str)  -->   type: dict(str)

from plugins.

ohinckel avatar ohinckel commented on July 28, 2024

I though it was working with both configuration setup - but I was wrong. In the meantime I change it back again to list(str) for the connect parameter.

But I want to use type dict for the new database parameter (which is the same, but not a list of strings instead it's a dict). Currently it doesn't work becase I get an error when using the database parameter with type dict or dict(foo) and configuration like

database:
  ...
  database:
    host: somehost
    user: username
    passwd: password
    db: databasename
    port: 3306

What type should be used in plugin.yaml to support such a structure?

from plugins.

psilo909 avatar psilo909 commented on July 28, 2024

@msinn told me on gitter, that multi layered configurations are not supported. so this could be a general problem for your approach.

where i succeeded was putting the values in '...' and setting a dict(str). but then the port makes problems when doing the database connection

from plugins.

msinn avatar msinn commented on July 28, 2024

I think structuring connection strings in a dict ist not a good idea. It puts the burden off building a valid connection string on the database plugin. Connection strings can be quite tricky (depending on the database system) and we don't know all the options someone might want to use to connect to his database.

from plugins.

ohinckel avatar ohinckel commented on July 28, 2024

I think structuring connection strings in a dict ist not a good idea.

At least it's more flexible: you only have to list the settings there if you want to use them which it's a generic approach, too.

I don't like the previous implementation because it has more "magic" implemented in extracting the connection parameter names and values. This could be just handled by the YAML structure itself.

I also don't like configuring one connection string (e.g. a DSN including parameters). We need to know how this is parsed (and maybe which parameters are supported by which driver implementation).

The plugin is based on the DB-API2 specification which is describing a connect() method which will get (currently) simple parameters (string, int, etc.) to setup a connection:

As a guideline the connection constructor parameters should be implemented as keyword parameters for more intuitive use and follow this order of parameters:

So mapping these simple keyword parameters dict makes sense to me.

from plugins.

msinn avatar msinn commented on July 28, 2024

At least it's more flexible: you only have to list the settings there if you want to use them which it's a generic approach, too.

It might work von sqlite and MySQL but I know of connection strings for other database systems (e.g. for Oracle DB) that won't fit in this dict structure very well.

If DB-API2 keeps track of this, it is fine with me. I was not aware, that DB-API2 does the magic to make a connection string that the database system understands.

from plugins.

ohinckel avatar ohinckel commented on July 28, 2024

I also know connection strings, but the DB-API specifies a connect() method which will have parameters. For the moment it doesn't matter if the parameter is a connection string (like a full DSN including parameters) or if they are specified as different parameters. But still both are supported when using a dict structure:

database:
  ...
  database:
    host: somehost
    user: someuser
    ...

vs.

database:
  ...
  database:
    dsn: somehost:port/dbname?param1=value&param2=other

The arguments depends on the implementation of connect() method of the driver. As long as only simple types are used as parameters (like str for a DSN string or str and int for host and port argument) it will work.

Further it is not very different from the current implementation: it also does only support these simple types (only the format is different: - host:name or for DSN - dsn:somedsn), but is an own implementation instead of relying on the YAML parser. I would prefer to use the YAML library to parse this instead of using an own implementation.

But currently the second level structure seems not to be supported. Will this be changed in the future? If not we need to stick to the current implementation.

But I also saw types using list(dict(str)), which looks like a nested structure (like in avm plugin, but it's in plugin function parameters not in common plugin parameters - seems to behave differently in handing the type attribute).

from plugins.

msinn avatar msinn commented on July 28, 2024

The metadata for Plugin Parameters (and Item Attributes) at the moment don't do type checking on more complex data structures. I am not sure (code was written some time ago): It might work if you specify dict instead of dict(str).

dict(str) is not supported and would not be very clear:

  • is the key a str?
  • is the value a str?
    and how to define more complex dicts?

Just using dict and knowing there is no data type checking when loading the parameters would leave room to read in dicts and do the type checking in the Plugin.

from plugins.

ohinckel avatar ohinckel commented on July 28, 2024

Just using dict and knowing there is no data type checking when loading the parameters would leave room to read in dicts and do the type checking in the Plugin.

I would vote to support this. So we can use a YAML structure for different stuff too and the plugin has to decide how to deal with the data.

Alternatively we could implement top-level items prefixed with "connect_" to indicate that these values are connection parameters. But we can not declare such fields in plugin.yaml - so maybe not the best solution.

My prefered way would be to just use dict. But this is causing a metadata error (as mentioned in the comment earlier):

But the metadata library issues an error since the test on the correct type only checks for a dict and not (additionally) for an OrderedDict which is created by the YAML parse - I guess.

2020-01-05  13:04:53 ERROR    lib.metadata      plugin 'database': Found invalid value 'OrderedDict([('host', 'XXX'), ('user', 'XXX'), ('passwd', 'XXX'), ('
db', 'XXX'), ('port', 'XXX')])' for parameter 'database' (type dict) in /etc/plugin.yaml, using default value '{}' instead

Just if we want to support type specifier for subtypes we could use the following syntax (similar to list:

  • dict - dict without knowing the type for key and type for value
  • dict(type) - dict with value type specified, e.g. dict(str)` will contain a dict with string values
  • dict(keytype|valuetype) - dict with given type for key and given type for value, e.g. dict(str|int) will contain a dict where keys are strings and values are integers
  • dict(keytype1,keytype2|valuetype1,valuetype2) - a dict with first key is of type keytype1, second key is of keytype2 and first value is of valuetype1 and second value of valuetype2, e.g. dict(str,int|str,str) - a two-value dict where first key is string, second is int and value of both items will be string (omitted types will use the last type - as it is working for list(str,int) too)

(we could also use a different delimiter instead of |, e.g. :, ; or /)

from plugins.

msinn avatar msinn commented on July 28, 2024

@ohinckel

My prefered way would be to just use dict. But this is causing a metadata error

No, the error is not from the definition dict but the actual /etc/plugin.yaml contains a list -> The error message is correct!
Have a look at OrderedDict([('host', 'XXX') Do you see the square bracket?
So: lib metadata could not convert a list (of tuples) to an ordered dict.

The error should disappear when the /etc/plugin.yaml data is corrected.

from plugins.

ohinckel avatar ohinckel commented on July 28, 2024

Why is it not able to convert it into a dict? Of course the output of an OrderedDict will dump the content as a list of tuples, but dealing with an OrderedDict should be (mostly) the same as dealing with a dict:

>>> import collections
>>> o=collections.OrderedDict()
>>> o["first"]="value"
>>> o["second"]="other"
>>> o
OrderedDict([('first', 'value'), ('second', 'other')])
>>> [key for key in o]
['first', 'second']
>>> [o.get(key) for key in o]
['value', 'other']
>>> dict(o)
{'second': 'other', 'first': 'value'}
>>> d={'second': 'other', 'first': 'value'}
>>> [key for key in d]
['second', 'first']
>>> [d.get(key) for key in d]
['other', 'value']
>>> 

The error message shows an error in /etc/plugin.yaml which does not exists in my system. I guess it mean the plugin.yaml in my SHNG installation - which is somewhere else. But this contains the following (no configuration as list at all):

db:
    class_name: Database
    class_path: plugins.database
    instance: dblog
    cycle: 20
    precision: -1
    driver: pymysql
    database:
      host: dbhost
      user: dbuser
      passwd: dbpasswd
      db: dbname
      port: 3306

from plugins.

msinn avatar msinn commented on July 28, 2024

Ok, then you are out of luck.

To handle it, we would need an special parser for etc/plugin.yaml and etc/module.yaml which interprets dicts.

At the moment the parser is used for all config files (.CONF or .YAML) including item definitions. For an item definition the parser could not distinguish between an item having a parameter of type dict and an item having a subitem. That is, because the config file parser doesn't know about the metadata. The metadata is applied only at the moment shng is initializing a plugin.

from plugins.

ohinckel avatar ohinckel commented on July 28, 2024

Ok, then you are out of luck.

That's bad, but I reverted the changes to the plugin is working as before.

[...] the parser could not distinguish between an item having a parameter of type dict and an item having a subitem.

Long time ago I asked to introduce a configuration object which could also handle this situation.

To fix this issue I updated the documentation to put the settings into quotes (just in case one is using them like a YAML structure).

from plugins.

msinn avatar msinn commented on July 28, 2024

Long time ago I asked to introduce a configuration object which could also handle this situation.

But the configuration object would not solve the problem that has its bases in the structure of the item configuration definition (regardless if .YAML or .CONF is used). I had to work around that when introducing the public properties of items as well.

To introduce dicts as datatype in items we have to have a fundamental change in the way item configurations are stored. That would leave a lot of users behind, especially those still using .CONF files.

from plugins.

ohinckel avatar ohinckel commented on July 28, 2024

At least it helps when parsing different configuration files. Of course it does not help for items configuration, since it's still depend on the dict structure for child items. But for (all) other configuration files it could help.

Are there still users using .CONF files? I thought this was deprecated years ago already.

To introduce dicts as datatype in items we have to have a fundamental change

Could be easily adapted by introducing a "child" element which contains child items. Migration script would also be easy.

But thanks anyway for you input. ATM I don't see any good solution for this.

from plugins.

bmxp avatar bmxp commented on July 28, 2024

As of current documentation support of *.CONF files is deprecated but supported until being dropped with Release 2.0.

from plugins.

msinn avatar msinn commented on July 28, 2024

When reading the forum, you see still a lot of questions with examples given as .CONF files

from plugins.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.