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

Underscore('_') removal too arbitrary in _safe_attr #87

Closed
pwwang opened this issue May 16, 2019 · 5 comments
Closed

Underscore('_') removal too arbitrary in _safe_attr #87

pwwang opened this issue May 16, 2019 · 5 comments
Labels
4.0 Feature planned for major 4.0 releasee bug
Milestone

Comments

@pwwang
Copy link
Contributor

pwwang commented May 16, 2019

Consider this code:

from box import Box
b = Box(_out = 'preserved')
b.update({'out': 'updated'})
# expected:
# {'_out': 'preserved', 'out': 'updated'}
# observed:
# {'_out': 'updated'}
# out lost

I thought this was designed intentionally at the beginning. However, when I digged into the code, I found the problem was due to https://github.com/cdgriffith/Box/blob/master/box.py#L151

    out = ''
    for character in attr:
        out += character if character in allowed else "_"
    out = out.strip("_")  <- L151

This arbitrarily removed all underscores in the key, even the originally assigned ones.
I won't say this is a bug, but really confusing.

Maybe we could use some unique characters to replace the characters that are not allowed, which will be removed later.

Say:

    out = ''
    for character in attr:
        out += character if character in allowed else "@"
    out = out.strip("@")
@cdgriffith cdgriffith added the bug label May 30, 2019
@cdgriffith
Copy link
Owner

Thanks for pointing this out!

I am actually going to modify the logic a bit so it is safer all around

    out = ''
    starting_chr = ''
    for character in attr:
        if starting_chr:
            out += character if character in allowed else "_"
        elif character in allowed:
            starting_chr = character
            out += character

    out = out.rstrip("_")

@pwwang
Copy link
Contributor Author

pwwang commented May 31, 2019

Will this overwrite the keys like out_ by out?

@cdgriffith
Copy link
Owner

Gah, good point. Need to fix it from that side as well.

@cdgriffith
Copy link
Owner

After thinking about it some more, this is going to have to hold off until a major release. It is a bug fix, however any exiting code that uses entities like these would break on update. I am actively working on 4.0 and will slate it for that.

@cdgriffith cdgriffith added this to the 4.0 milestone Jun 4, 2019
@cdgriffith cdgriffith added the 4.0 Feature planned for major 4.0 releasee label Jun 4, 2019
@cdgriffith
Copy link
Owner

Fixed in 4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.0 Feature planned for major 4.0 releasee bug
Projects
None yet
Development

No branches or pull requests

2 participants