Write approaches for Yacht

[safwansamsudeen]

As per the blog post on Freeing our Maintainers, the community contributions pause has an exception for writing approaches. This is for Yacht.

Let me know your suggestions. This is my first time, so my apologies if there are any errors in the format.

[BethanyG]

Hi @safwansamsudeen - Thanks for submitting this. 🙂

As with other areas of documentation on the Python track, we ask that you do not use single letter variable names in code that is supposed to serve as an exemplar for students. Yes - even PEP8 does so, but we do not. Please change/remove the places you use single letter variable names.

Additionally, (arguments in the forum and other places not withstanding), we follow PEP8 for coding style. PEP8 specifically calls out assigning variable names to lambdas as incorrect:

So while many many many solutions do indeed use that convention, we ask that you do not do so in documents that instruct students. While you can make reference to the solution that uses them, calling it idiomatic is really not correct in these circumstance.

I'll review further after those changes are made.

[safwansamsudeen]

I'll correct those.

Before that, I'm trying to install all the packages, and am getting this dependency conflict error:

The conflict is caused by:
    The user requested astroid<=2.12.0
    pylint 2.17.4 depends on astroid<=2.17.0-dev0 and >=2.15.4
    The user requested astroid<=2.12.0
    pylint 2.17.3 depends on astroid<=2.17.0-dev0 and >=2.15.4
    The user requested astroid<=2.12.0
    pylint 2.17.2 depends on astroid<=2.17.0-dev0 and >=2.15.2
    The user requested astroid<=2.12.0
    pylint 2.17.1 depends on astroid<=2.17.0-dev0 and >=2.15.0

Is it a problem on my side or something erroneous with the requirements.txt?

[BethanyG]

I'm not sure why you are installing requirements. Writing approaches don't need any. Was there something else you needed to do?

[safwansamsudeen]

I thought I needed it to format files - and in the longer run, if I contribute to exercises, I'd need this.

[BethanyG]

I thought I needed it to format files - and in the longer run, if I contribute to exercises, I'd need this.

One step at a time. 😄 We'll probably want to have a chat first before any exercise contributions - and we can figure out the dev environment then. Not all of our tooling is represented in this repo.

But I will also take a look later today/this week and see what's up with the file. I know I changed the requirements as I was upgrading to Python 3.11, and that I ran into some versioning issues. But IIRC, that file is used for some of our tooling, so ought to work. I'll re-test things.

I need to do a sweep of the Python Track docs anyways - I suspect I didn't catch all the places that needed updating when we upgraded. 😬

[BethanyG]

Suggested change

	The repetitive code is minimized using a seperate function `digits` that returns a function (closure). For more information on this approach, read [this document][approach-functions].
	For more information on this approach, read [this document][approach-functions].

[BethanyG]

Hi @safwansamsudeen -- apologies for taking so long to review this. There are a few things here:

1. Our format for markdown documents (see the layout section here) is to have one sentence per line for text that's not a code block. I caught some of these when making suggestions, but I didn't catch them all -- so you will probably need to go back over the documents, and ensure the one-sentence-per-line rule.
2. I changed your if approach code. I was very uncomfortable with the code example, as it read very convoluted -- as if you were trying to push lambdas over if statements.
3. I did make a lot of suggestions. You don't have to accept them all - but I do want to remove any language that implies assigning names to lambdas as idiomatic.

I'm not going to require that you make all these changes to approve the PR -- but I do need to have one-sentence-per-line, and the lambda-related edits. I might also suggest you do an approach using structural pattern matching (new in 3.10!) - it is a nice medium between one-line functions and using lots of ifs. Although if you don't want to do that, I can come back and add it later.

[safwansamsudeen]

Hi @BethanyG, thanks for the suggested changes!

I think I've implemented everything, and caught one or two other instances of multiple sentences per line. Let me know if I've left anything out. In a few cases, I've asked doubts but implemented the suggestion anyway.

Sorry for the delay: I'm currently on vacation, will get resume from next week! Pattern matching sounds good, I'll implement it then.

[BethanyG]

Hi @safwansamsudeen -- I noticed you re-tagged this for review, but haven't made additional changes.

I though I would wait until you were back from vacation and added the final pattern matching approach before going over this one final time. Did you have additional questions or problems? Please let me know - thanks!

If you'd rather add the pattern matching in a follow-on, let me know and we can merge this as it stands.

[safwansamsudeen]

Hi @BethanyG,

How does this look? And a GH doubt: you've said "re-tagged" for review - are you automatically tagged when I make changes, or should I click on the reload icon in the reviewers section to notify you? That's what I've been doing.

[BethanyG]

@safwansamsudeen - I get pinged for every change pushed, as well and being pinged for review. And that's OK!

But in this case, it didn't look like you'd made significant changes, and your message also mentioned you were on vacation, so I decided I'd wait. If you think something has slipped through the cracks, or you'd like me to look at it sooner rather than later, you can always re-ping me for review.

But once I've commented or reviewed on anything (bug, issue, PR), I will get a notification from GH for any comments or changes (even when the issue is closed!). 😄

I don't have time in the next few days to go over the SPM approach in detail - apologies for that. Hopefully, I will get to it before the weekend.

But one quick comment: you can combine cases for this I think. Big Straight and Little Straight can go together, as can Full House and Choice. You can also return 0 from each case (using the ternary form) and avoid the zero fall-through case. Finally, if you make the single digits the fall-through case, you can avoid doing the explicit 1|2|3|4|5|6 check.

Thanks again for your work on this.

[safwansamsudeen]

Ah... OK, I won't retag for review, apologies :).

I don't fully get how I could combine the cases while maintaining good style, so I think I'll wait for your review - no hurry there!

Returning 0 in each case using ternary - isn't that unnecessary repetition? I thought the wildcard was meant for cases like this - but I haven't used SPM much, so feel free to make the change if you think it's better.

[BethanyG]

@safwansamsudeen --

Both

def score(dice, category): 
    roll = sorted(set(dice))
    
    match category:
        case 'big' | 'little' :
            straight = ((category == 'little' and roll == [1, 2, 3, 4, 5])
                        or (category == 'big' and roll == [2, 3, 4, 5, 6]))
            return 30 if straight else 0
        
        case 'full_house' | 'choice' : 
            full = any(dice.count(item) == 3 for item in roll)
            return sum(dice) if category == 'choice' or full else 0
        
        case 'four_of_a_kind' if (dice[0] == dice[3]) or (dice[1] == dice[4]):
            return 4 * dice[1] 
        
        case 'yacht' if len(roll) == 1 : 
            return 50
        
        case _:
            return sum(item for item in dice if item == category)

As well as

def score(dice, category): 
    
    match (category, roll := sorted(set(dice))):
        case ('full_house', _) | ('choice', _) : 
            full = any(dice.count(item) == 3 for item in roll)
            return sum(dice) if category == 'choice' or full else 0
    
        case ('big', [2, 3, 4, 5, 6]) | ('little', [1, 2, 3, 4, 5]) :
            return 30

        case ('four_of_a_kind', _) if (dice[0] == dice[3]) or (dice[1] == dice[4]):
            return 4 * dice[1] 

        case ('yacht', _) if len(roll) == 1 : 
            return 50

        case _:
            return sum(item for item in dice if item == category)

Worked for me. It would appear that as the tests are written for this, only the first case really needs a ternary. Cheating a bit, I know - and the second example uses far too many _ wildcards for my liking - but there you have it.

All that being said, I am fine keeping your example as-is, if you think it reads better.

[BethanyG]

@safwansamsudeen - Thanks for this! I am going to approve, and if you'd like to change the SPM example - great - but otherwise, we can merge.

[safwansamsudeen]

Thanks for the review!

I feel that the currently committed example reads better, and sticks to the SPM approach in core instead of adding other details for the student. Even more significantly, it's either getting round the tests, like you say, or adding a ternary to each statement, which becomes repetitive.

If this is fine, I think we can merge!

[safwansamsudeen]

Thanks for merging!

Not sure if it's a bug... but I've got 60 reputation point for thIs PR - one for opening it (getting it merged, that is), one for the approach itself, and one for each individual approach. Is this to be expected? (If it is, I'm thrilled!)

[safwansamsudeen]

A duplication of one of the line exists: #3424