Skip to content

Updating ftc.py courtesy of @j5155, as well as fixing a past bug with datetime from a previous PR of mine#460

Merged
tweirtx merged 14 commits intoFRCDiscord:masterfrom
skruglov2023:patch-1
Apr 1, 2024
Merged

Updating ftc.py courtesy of @j5155, as well as fixing a past bug with datetime from a previous PR of mine#460
tweirtx merged 14 commits intoFRCDiscord:masterfrom
skruglov2023:patch-1

Conversation

@skruglov2023
Copy link
Copy Markdown
Contributor

Merging changes from ftc-dozer2-0/Dozer2.0#13 to here, including the correct file structure for this bot Added ftcscout (much better ftc stats) to the ftc commands Fixed datetime in ftc.py
Removed toa, since toa pulls from ftc-events anyways. Deleted the toa section from config generation in main.py Fixed /stats issue with datetime (different level import between dozer2 and here)

To here, including the correct file structure for this bot
Added ftcscout (much better ftc stats) to the ftc commands
Fixed datetime in ftc.py
Removed toa, since toa pulls from ftc-events anyways. 
Deleted the toa section from config generation in main.py
Fixed /stats issue with datetime (different level import between dozer2 and here)
Comment thread dozer/cogs/ftc.py Outdated
@skruglov2023 skruglov2023 requested a review from devyntk January 2, 2024 23:59
Comment thread dozer/cogs/ftc.py
Comment thread dozer/cogs/info.py
Comment thread dozer/__main__.py
@guineawheek
Copy link
Copy Markdown
Collaborator

small nit but the reason why import datetime is used over from datetime import datetime is that datetime provides other useful classes like timedelta, although i guess you could also do from datetime import datetime, timedelta

Comment thread dozer/cogs/ftc.py Outdated
Comment thread dozer/cogs/ftc.py
e.add_field(name='Website', value=website or 'n/a')
e.add_field(name='Team Info Page', value=f'https://ftcscout.org/teams/{team_num}')

if sres.status != 404:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh i would separate out OPR information to a separate command (maybe %ftc opr). I don't think it really belongs in the general command (and could get kinda toxic really quickly)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How will this become toxic?
Not sure how making it a dedicated command would help if so
This suggestion received overwhelming support in FTC discord (29 up 0 down), maybe discuss it there? You might be right I'm just not sure I understand what you mean

Copy link
Copy Markdown
Collaborator

@guineawheek guineawheek Jan 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggestion received overwhelming support in FTC discord (29 up 0 down), maybe discuss it there? You might be right I'm just not sure I understand what you mean

First off, you said it could happen, not be something that gets tacked onto every team's entry in the default command.

How will this become toxic?

Having OPR be something the bot pulls by default when calling %ftc could quickly become a way to invalidate takes that users make. It could indirectly become a way of saying "your opinion is invalid because your team is bad"

Copy link
Copy Markdown
Contributor Author

@skruglov2023 skruglov2023 Jan 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
Seems like the main concern is specifying where the data comes from (ftc-events/ftcscout) rather than toa (command-name related)
/toa is probably going to remain an alias for the /ftc team command due to how used it appears to be still.
Maybe we can keep the default OPR for now and get some feedback from the wider community, and adjust based off observations and usage?

Comment thread dozer/cogs/ftc.py
@j5155
Copy link
Copy Markdown
Contributor

j5155 commented Jan 17, 2024

Will try to address those issues tonight @guineawheek

@skruglov2023
Copy link
Copy Markdown
Contributor Author

Believe the latest comments have been resolved via latest merge with this patch branch

@j5155
Copy link
Copy Markdown
Contributor

j5155 commented Mar 18, 2024

OPR has been seperated out into it's own command, %ftc opr, aliased to %topr (alias broken in 142487a (#460) but should hopefully be fixed once stephan is free)
does this address your concerns @guineawheek ?

@guineawheek
Copy link
Copy Markdown
Collaborator

i think opr is fine separately

@skruglov2023
Copy link
Copy Markdown
Contributor Author

%topr and %ftc opr should now be up to date on this PR, as per @j5155's message

@j5155
Copy link
Copy Markdown
Contributor

j5155 commented Mar 19, 2024

This PR is ready to merge as far as my planned code changes go.
@devyntk looks like GitHub is still waiting for your review. We changed to f strings as you suggested, are there any other changes you would like?

@tweirtx tweirtx merged commit 88f8d2d into FRCDiscord:master Apr 1, 2024
@skruglov2023 skruglov2023 deleted the patch-1 branch May 2, 2024 15:25
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.

7 participants