Skip to content

Conversation

@behrtam
Copy link
Contributor

@behrtam behrtam commented Nov 22, 2015

Example solution for new proposed exercise exercism/problem-specifications#133
Will be updated later based on possible feedback there.

@Dog
Copy link
Contributor

Dog commented Nov 25, 2015

Under what case is your set going to have a length over 26? (assuming ascii_letters)

Isn't this exercise region specific due to the use of isalpha? I don't know much about region differences, so I'm going to use German characters in my example only because I'm more familiar with the language.

My understanding of isalpha is it uses the constant string.letters to determine how it should return. If we switched to a "German" region, I'm worried ä, ö, ü, ß will also be included in the set because string.letters is region dependent. Since the size of string.letters changed, this means we could potentially substitute the letter z for the letter ä and still pass.

This raises a few questions:

  • Should a German pangram include ä, ö, ü, ß?
  • Should we consider limiting to ascii_letters?

@behrtam
Copy link
Contributor Author

behrtam commented Nov 25, 2015

Good point! Yes we should limit it to ascii. The string method isalpha() is a bad choice here. 'abcdefghijklmnopqrstuvwΣΩδ' as input would return true while it clearly is missing 'xyz'.
We should use ? in ascii_lowercase instead of isalpha().

>>> from string import ascii_lowercase
>>> pan = lambda s: len(set(char for char in s.lower() if char in ascii_lowercase)) == 26
>>> pan('abcdefghijklmnopqrstuvwxyz')
True
>>> pan('abcdefghijklmnopqrstuvwΣΩδ')
False
>>> pan('abcdefghijklmnopqrstuvwΣΩδxyz')
True
>>>

@behrtam
Copy link
Contributor Author

behrtam commented Nov 25, 2015

We could even get rid of the len/set combination. This is fare more readable.

>>> pan = lambda s: all(char in s.lower() for char in ascii_lowercase)
>>> pan('abcdefghijklmnopqrstuvwΣΩδxyz')
True
>>> pan('abcdefghijklmnopqrstuvwΣΩδ')
False
>>>

@Dog
Copy link
Contributor

Dog commented Nov 25, 2015

I really like the solution with all. I think its ready for merging otherwise.

behrtam added a commit that referenced this pull request Nov 25, 2015
pangram: Implemented new exercise
@behrtam behrtam merged commit 7c16c4e into exercism:master Nov 25, 2015
@behrtam behrtam deleted the implement-pangram branch February 8, 2016 10:55
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.

2 participants