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 #3156: Modify Dictionary implementation to match docs #3211

Closed
wants to merge 3 commits into from

Conversation

mwichmann
Copy link
Collaborator

@mwichmann mwichmann commented Oct 9, 2018

Documentation for Dictionary(): "Returns a dictionary object containing copies of all of the construction variables in the environment. If there are any variable names specified, only the specified construction variables are returned in the dictionary." However, in the existing implementation, if called with no arguments it directly returns the internal dictionary of the construction environment, the one it uses to implement its dictionary-like behavior (matches docs); if a single key argument is passed it returns the value for that key; if multiple keys are passed it returns a list of the matching values. In neither of the latter two case does it return a dict object. This change in Environment.py aligns the behavior with the docs.

Since all the tests are coded to the actual behavior, rather than the documented behavior, no tests demonstrated any problem, which is only seen if a user themselves uses Dictionary as documented. All the tests which depend on the old behavior thus needed some adjustment. The normal pattern follows this example:

  env['FOO'] = 'foo'
  assert env.Dictionary('FOO') == 'foo'

which breaks when Dictionary returns a dict instead of the value. That can change to either of two following forms - the former is more like the origial intent, but the second pattern has been chosen because it looks less awkward, except for a few cases that seemed to be testing specifically the limiting of the returned Dictionary by giving it a specific number of keys:

  assert env.Dictionary('FOO')['FOO'] == "foo"
  assert env.Dictionary()['FOO'] == "foo"

A couple of internal tests define dummy classes which implement their own version of Dictionary. These needed updating as well. If the dummy took no args, it was left alone (BuilderTests.py, FSTests.py, NodeTests.py). If the dummy ignored args it still does so, but got a comment line to that effect. If it takes one key argument it builds a one-item dict (SubstTests.py, msvsTests.py), if it takes *args like the main Dictionary it got the change of the original (ProgTests.py), and if it took args but did something special (FortranTests.py, IDLTests.py) it was adjusted to be sure it was still returning a dict object. I'm not clear if the cases where a dummy Dictionary does not have the same signature as the real Dictionary are correct, but there are enough changes here already, we could look at that topic separately.

Fixes #3156
Signed-off-by: Mats Wichmann mats@linux.com

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated master/src/CHANGES.txt directory (and read the README.txt in that directory)
  • I have updated the appropriate documentation

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.5%) to 66.043% when pulling bcc0f27 on mwichmann:wip-Dictionary into e342d03 on SCons:master.

@coveralls
Copy link

coveralls commented Oct 12, 2018

Coverage Status

Coverage decreased (-6.6%) to 75.053% when pulling 0c78590 on mwichmann:wip-Dictionary into b4d710b on SCons:master.

@bdbaddog
Copy link
Contributor

Can you take a look at the travis failure?
https://travis-ci.org/SCons/scons/jobs/442316446
Strange that it only failed in the coverage run?

@mwichmann
Copy link
Collaborator Author

I looked earlier, and don't know what to say. It is the trickiest test change, the one where I had to resort to using a set to be able to check efficiently, but... it passes everything I've run it on here, and all the other CI builds, so I'm mystified at the moment. Is there anything different I should know about for that build?

Documentation for Dictionary(): "Returns a dictionary object containing
copies of all of the construction variables in the environment. If
there are any variable names specified, only the specified construction
variables are returned in the dictionary." However, in the existing
implementation, if called with no arguments it directly returns the
internal dictionary of the construction environment, the one it uses
to implement its dictionary-like behavior (matches docs); if a single
key argument is passed it returns the value for that key; if multiple
keys are passed it returns a list of the matching values. In neither
of the latter two case does it return a dict object.  This change in
Environment.py aligns the behavior with the docs.

Since all the tests are coded to the actual behavior, rather than
the documented behavior, none demonstrated any problem, which is
only seen if a user themselves uses Dictionary() as documented.
All the tests which depend on the old behavior thus needed some
adjustment. The normal pattern follows this example:

  env['FOO'] = 'foo'
  assert env.Dictionary('FOO') == 'foo'

which breaks when Dictionary returns a dict instead of value. That
can change to either of two following forms - the former is more
like the origial intent, but the second pattern has been chosen
because it looks less awkward, except for a few cases that seemed
to be testing specifically the limiting of the returned Dictionary
by giving it a specific number of keys:

  assert env.Dictionary('FOO')['FOO'] == "foo"
  assert env.Dictionary()['FOO'] == "foo"

A couple of internal tests define dummy classes which implement their
own version of Dictionary.  These needed updating as well.  If the
dummy took no args, it was left alone (BuilderTests.py,  FSTests.py,
NodeTests.py). If the dummy ignored args it still does so, but got a
comment line to that effect.  If it takes one key argument it builds a
one-item dict (SubstTests.py, msvsTests.py), if it takes *args like the
main Dictionary it got the change of the original (ProgTests.py), and if
it took args but did something special (FortranTests.py, IDLTests.py)
it was adjusted to be sure it was still returning a dict object. I'm
not clear if the cases where a dummy Dictionary does not have the same
signature as the real Dictionary are correct, but there are enough
changes here already, we could look at that topic separately.

Fixes SCons#3156
Signed-off-by: Mats Wichmann <mats@linux.com>
mwichmann added a commit to mwichmann/scons that referenced this pull request Apr 12, 2019
Since the last time this was built, Travis and Appveyor run
more tests, and that turned up some missed locations that
needed updating.

Signed-off-by: Mats Wichmann <mats@linux.com>
mwichmann added a commit to mwichmann/scons that referenced this pull request Apr 12, 2019
Since the last time this was built, Travis and Appveyor run
more tests, and that turned up some missed locations that
needed updating.

Signed-off-by: Mats Wichmann <mats@linux.com>
mwichmann added a commit to mwichmann/scons that referenced this pull request Apr 12, 2019
Since the last time this was built, Travis and Appveyor run
more tests, and that turned up some missed locations that
needed updating.

Signed-off-by: Mats Wichmann <mats@linux.com>
mwichmann added a commit to mwichmann/scons that referenced this pull request Apr 13, 2019
Since the last time this was built, Travis and Appveyor run
more tests, and that turned up some missed locations that
needed updating.

Signed-off-by: Mats Wichmann <mats@linux.com>
Since the last time this was built, Travis and Appveyor run
some new tests (lex) which needed the Dictionary usage update.
Also adjust some of the asserts - fixed the set test for
fetching more than a single key-value pair via Dictionary,
and added the expression to raise to several assert statements.

Signed-off-by: Mats Wichmann <mats@linux.com>
@mwichmann
Copy link
Collaborator Author

I can withdraw this PR so it doesn't clutter up the list. See issue #3156 for discussion.

@mwichmann
Copy link
Collaborator Author

Withdrawn in favor of #3423

@mwichmann mwichmann closed this Aug 25, 2019
@mwichmann mwichmann deleted the wip-Dictionary branch August 25, 2019 19:51
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.

Dictionary() implementation does not match documentation
3 participants