Navigation Menu

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

Fixes #1298 null value keybindings #1301

Merged
merged 2 commits into from Jun 29, 2018
Merged

Conversation

KSchopmeyer
Copy link
Collaborator

@KSchopmeyer KSchopmeyer commented Jun 22, 2018

This is ready for review and merge. See commit for details

@KSchopmeyer KSchopmeyer added this to the 0.13.0 milestone Jun 22, 2018
@KSchopmeyer KSchopmeyer self-assigned this Jun 22, 2018
@coveralls
Copy link

coveralls commented Jun 24, 2018

Coverage Status

Coverage decreased (-1.0%) to 83.333% when pulling 7812c53 on ks/#1298-null-keybindings into 9083c1e on master.

@andy-maier
Copy link
Contributor

It seems to me there is a general issue that we (I) don't understand yet, and that applies to all config variables:

It seems that pywbem.config.XXX and pywbem.XXX are two different objects, even though the names from pywbem.config are imported into pywbem.

Given that we document to use the config variables in the pywbem namespace, but they are documented to be in the pywbem.config namespace, this creates a significant chance for users to run into that issue.

We need to do some basic research on this and find a general solution.

@KSchopmeyer
Copy link
Collaborator Author

I agree. One more discussion item on the agenda.

@andy-maier andy-maier changed the title WIP fixes #1298 null value keybindings [WIP] Fixes #1298 null value keybindings Jun 28, 2018
@KSchopmeyer
Copy link
Collaborator Author

We agreed to change the access methods for config. We will commit this change and andy will do the general change.

@andy-maier
Copy link
Contributor

I created issue #1302 for the general config variable update issue. This PR here can go on to set and read its new config variable through the pywbem.config namespace.

@@ -253,6 +253,7 @@ class (the parent object) has a list of CIM properties, CIM methods and CIM

from . import cim_xml
from .config import DEBUG_WARNING_ORIGIN, SEND_VALUE_NULL
import pywbem.config
Copy link
Contributor

Choose a reason for hiding this comment

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

This should import from ., not from pywbem, to make sure it remains a relative import. In other words, this would become:

from . import config

and your new config variable would be accessed as:

config.IGNORE_NULL_KEY_VALUE

And just to be on the safe side, since you mentioned issues with from ... import ...:

>>> from pywbem import config
>>> import pywbem
>>> id(pywbem.config)
4439529464
>>> id(config)
4439529464

Both pywbem.config and config are bound to the same module object, so I don't see an issue with from ... import ....

Copy link
Contributor

@andy-maier andy-maier left a comment

Choose a reason for hiding this comment

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

See my comments, plus remove the PKG-INFO file from the commit.
Otherwise, I'm fine.

pywbem/config.py Outdated
@@ -93,3 +93,17 @@
#:
#: *New in pywbem 0.12.*
SEND_VALUE_NULL = True

#: Backward compatibility option that controls creating CIMInstanceNames with
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using plural on a class name, use a link on the class name:

... creating :class:`~pywbem.CIMInstanceName` objects with ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DONE

pywbem/config.py Outdated
#: null values for keybindings.

#: :term:`DSP0004`, section 7.9 specifically forbids key properties with
#: no values but because pywbem has always allowed this, adding the code to
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of saying "with no values", I suggest to say "that are NULL".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DONE

pywbem/config.py Outdated
#: no values but because pywbem has always allowed this, adding the code to
#: disallow None as a keybinding value is an incompatible change.
#:
#: * True: Pywbem ignores None as a value when keybindings are defined.
Copy link
Contributor

Choose a reason for hiding this comment

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

It does not "ignore" None in this case, but "tolerates" it.

pywbem/config.py Outdated
#: disallow None as a keybinding value is an incompatible change.
#:
#: * True: Pywbem ignores None as a value when keybindings are defined.
#: * False (default): Pywbem generates ValueError when keybindings are created
Copy link
Contributor

Choose a reason for hiding this comment

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

Use back quotes around True, False, None.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DONE

pywbem/config.py Outdated
#: disallow None as a keybinding value is an incompatible change.
#:
#: * True: Pywbem ignores None as a value when keybindings are defined.
#: * False (default): Pywbem generates ValueError when keybindings are created
Copy link
Contributor

Choose a reason for hiding this comment

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

It does not "generate" ValueError, but "raises" ValueError. Also, use a link for ValueError.

Adds a new config variable IGNORE_NULL_KEY_VALUE that controls whether
CIMInstanceName keybindings can set None as a key value.

Adds test for this variable and adds comment to changes.rst.

Added import pywbem.config to cim_obj.py to that we always get the
original variable, not the copy made by from .config import ...
That works.
Marked this whole thing for Discussion
@KSchopmeyer KSchopmeyer changed the title [WIP] Fixes #1298 null value keybindings Fixes #1298 null value keybindings Jun 29, 2018
@KSchopmeyer KSchopmeyer dismissed andy-maier’s stale review June 29, 2018 16:26

Made changes for your comments.

@andy-maier andy-maier merged commit b0a4964 into master Jun 29, 2018
@andy-maier andy-maier deleted the ks/#1298-null-keybindings branch June 29, 2018 22:19
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.

None yet

3 participants