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
Merged

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

merged 1 commit into from Feb 16, 2017

Conversation

adrientetar
Copy link
Contributor

No description provided.

@anthrotype
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

you're right.

@anthrotype anthrotype merged commit 18e07fb into robotools:master Feb 16, 2017
@adrientetar adrientetar deleted the rm-list branch February 16, 2017 16:45
@anthrotype
Copy link
Member

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants