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

Table removal isn't really a removal #205

Closed
codejedi365 opened this issue Jul 3, 2022 · 2 comments · Fixed by #206
Closed

Table removal isn't really a removal #205

codejedi365 opened this issue Jul 3, 2022 · 2 comments · Fixed by #206
Labels
bug Something isn't working

Comments

@codejedi365
Copy link

As an extension of related issue #204, the alternative to del, remove(), of a table has unexpected results for multi-tiered tables.

Based on the quickstart.rst, one can use the remove() function to remove a table from a TOMLDocument. This is inaccurate. It is not an in-place operation as one would expect but instead returns a new copy of the TOMLDocument with the desired field removed. The original object is untouched.

With this querk, it means that when removing a child table of a larger TOMLDocument one must replace the parent table with a setattr() action (or other python variant). This should at least be documented or an alternative created because this does not follow most remove conventions.

Recommendation

  1. (Preferred) Deprecate the remove() function and instead provide a pop() which most similarly matches a pythonic Dictionary. As with the internals of how the json equivalent is rendered as a dictionary, this mirrors the expectations of Python programmers. The Dictionary.pop() is an in-place operation as it removes the desired element based on the provided key and provides it as the returned value.

  2. Update the remove() function to be an in-place operation which follows the pythonic List remove() feature. It returns None but updates the TOMLDocument internal data structure.

Example

The example is here to illustrate an incorrect result and not for debate on if this should be done or not.

Imported pyproject.toml

[project]
name = "examplepkg"

[project.optional-dependencies]
dev = [
    "python-semantic-release"
]

Example code which illustrates expected/actual removal operations

"""Script to strip optional-dependencies from pyproject configuration."""
import json, tomlkit

with open('pyproject.toml', 'rb') as fd:
    conf = tomlkit.load(fd)

if conf.get('project', {}).get('optional-dependencies', None) is not None:
    conf['project'].remove('optional-dependencies')

print("Removal is not an in-place action. This result is unchanged:")
print(json.dumps(conf, indent=2))
print(tomlkit.dumps(conf))

# Neither are updated results

Expected Result

JSON output (expected)

{
  "project": {
    "name": "exampleprj"
  }
}

TOML output (expected)

[project]
name = "examplepkg"

Actual Result

JSON output (actual)
Result: Failure

{
  "project": {
    "name": "exampleprj",
    "optional-dependencies": {
      "dev": [
        "python-semantic-release"
      ]
    }
  }
}

TOML output (actual)
Result: Failure

[project]
name = "examplepkg"

[project.optional-dependencies]
dev = [
    "python-semantic-release"
]

Current method of performing an sub-table removal

"""Script to strip optional-dependencies from pyproject configuration."""
import json, tomlkit

with open('pyproject.toml', 'rb') as fd:
    conf = tomlkit.load(fd)

if conf.get('project', {}).get('optional-dependencies', None) is not None:
    # currently expected implementation for sub-table removal
    conf['project'] = conf['project'].remove('optional-dependencies')

print("using __setattr__() to capture return value of remove(), completes a removal of sub-table:")
print("=====================")
print(json.dumps(conf, indent=2))
print(tomlkit.dumps(conf))
@frostming frostming added the bug Something isn't working label Jul 4, 2022
@frostming
Copy link
Collaborator

frostming commented Jul 4, 2022

First, I can't reproduce the issue:

import json, tomlkit

conf = tomlkit.parse(
    """[project]
name = "examplepkg"

[project.optional-dependencies]
dev = [
    "python-semantic-release"
]
"""
)

if conf.get("project", {}).get("optional-dependencies", None) is not None:
    conf["project"].remove("optional-dependencies")

print("Removal is not an in-place action. This result is unchanged:")
print(json.dumps(conf, indent=2))
print(tomlkit.dumps(conf))

Outputs:

Removal is not an in-place action. This result is unchanged:
{
  "project": {
    "name": "examplepkg"
  }
}
[project]
name = "examplepkg"

Maybe your example is incorrect, because you said "multi-tiered tables"?

Second, there is .pop() on the table, table.pop(key) calls del table[key], which calls table.remove(key) under the hood.

@codejedi365
Copy link
Author

@frostming , Thank you for figuring out it was related to the issue 206 example. It was my fault when I stripped down the example test in this issue. I didn't realize it was because of the out of order table that the remove was failing. pop works! thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants