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

Layer: don't make a list out of glyph names #100

Merged
merged 1 commit into from Feb 16, 2017

Conversation

Projects
None yet
2 participants
@adrientetar
Copy link
Contributor

commented Feb 13, 2017

No description provided.

@adrientetar adrientetar force-pushed the adrientetar:rm-list branch from 85946c4 to 8a4170d Feb 13, 2017

@anthrotype

This comment has been minimized.

Copy link
Member

commented Feb 15, 2017

I guess you want to avoid the extra copy when converting the self._keys set into a list.
But this could create problems for existing code that relies on keys() returning a list instead of a set.
Also note that defcon.Font.keys() method returns a list, as does ufoLib.GlyphSet.keys().
The thing is all these libraries were originally python2-only, where dict.keys() would return a list (copy) of the keys in a dictionary. When porting them to python3, we tried to keep the python2 behaviour.

@anthrotype anthrotype closed this Feb 15, 2017

@anthrotype anthrotype reopened this Feb 15, 2017

@anthrotype

This comment has been minimized.

Copy link
Member

commented Feb 15, 2017

sorry, I pressed the wrong button. I didn't mean to close the PR. I'm open to merge this if others say I'm worrying too much.

@adrientetar

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2017

But this could create problems for existing code that relies on keys() returning a list instead of a set.

What kind of code would rely on keys() being a list? The returned order is arbitrary, afaict.

Also note that defcon.Font.keys() method returns a list

This is an alias to font.layers[defaultLayer].keys() – this pull request changes it, afaict.

@anthrotype

This comment has been minimized.

Copy link
Member

commented Feb 16, 2017

you're right.

@anthrotype anthrotype merged commit 18e07fb into robotools:master Feb 16, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@adrientetar adrientetar deleted the adrientetar:rm-list branch Feb 16, 2017

@anthrotype

This comment has been minimized.

Copy link
Member

commented Feb 16, 2017

maybe the docstring should also be updated to explicitly say it now returns a set, for people who are used to get list from keys-like methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.