search

wcrasta / ESPN-Fantasy-Basketball

Free Fantasy Basketball tools for ESPN Leagues
https://espnfantasy.warrencrasta.com
MIT License
32 stars 15 forks source link

Create new route for showing if a player helps or hurts a team's category #28

Closed suyashm95 closed 1 year ago

suyashm95 commented 1 year ago

It's not letting me add you as a reviewer @wcrasta but take a look whenever you get the chance. Lmk if you have any questions

suyashm95 commented 1 year ago

image Here's a screenshot of it in action too!

suyashm95 commented 1 year ago

This feature aims to help teams get a better understanding of how each of their players affects their overall category performance. I hope that people use this view when thinking about trades or looking at potential category builds. The math may be slightly off (I have a comment in the cleanup_season_values_and_get_average_per_roster_spot function), but I figured it's better than nothing and it can be something we can improve once we have some more tools at our disposal.

wcrasta commented 1 year ago

Awesome! Thanks so much for adding this feature. I took a quick look at the screenshot and it looks great. Just commenting to acknowledge this PR and that I'm putting it on my backlog of things to do. I'll aim to review it this weekend (or earlier).

wcrasta commented 1 year ago

Hi @suyashm95, thanks again for your contribution! I read about a third of the PR and left some comments. Feel free to push back if you think the pros for how you implemented something outweigh the cons. After those comments are addressed, I'd be happy to review the rest of the PR.

The existing code in this repo isn't in great shape because I wrote it many years ago when I was new to programming. Still, I'd like to ensure that new code that's added is maintainable. I noticed that there's some duplicate code and big if-else statements in this PR. It would be great if you could refactor the code to avoid those. Many if-else statements are considered a code smell and can be refactored to use dictionaries. Let me know if you'd like my input/help with that!

suyashm95 commented 1 year ago

Hey @wcrasta , thanks for reviewing the PR! I was busy with stuff during the holiday season, but I have more time now to address your comments. Look for some updates here within the week

wcrasta commented 1 year ago

Cool, that sounds good to me!

wcrasta commented 1 year ago

Sorry for the late acknowledgement, but I'll aim to review the new changes this week! Thanks again for your contribution.

wcrasta commented 1 year ago

@suyashm95, I'm terribly sorry -- I've been preoccupied with other things and didn't review this PR in the timeframe that I said I would. I checked out your branch locally and tried running the code today, but there are some errors in the code:

cleaned_up_stats[stat_timeframe][category] = round(stats[i]['averageStats'][stats_map[category]], 3)
KeyError: '17'

Since it has taken so long for me to review the code, it wouldn't be fair to ask you to fix the code and resubmit for review. I thought about spending some time and fixing it myself, but I just have too many competing priorities at the moment. Given that my league hasn't used ESPN in a few years as well as knowing that ESPN can change their website at any time & completely break this project, I'm hesitant to spend time maintaining this project. I'm going to indicate that it's no longer maintained and archive it.

Again, I'm sorry that I couldn't see this PR through completion, but I'm grateful for you supporting & taking an interest in this project. Wishing you all the best!