Skip to content

AoC month changes#996

Merged
Den4200 merged 2 commits into
mainfrom
aoc-changes
Jan 1, 2022
Merged

AoC month changes#996
Den4200 merged 2 commits into
mainfrom
aoc-changes

Conversation

@ChrisLovering
Copy link
Copy Markdown
Member

Description

This allows AoC commands to be ran in January, along with limiting AoC join to only be ran in Nov, Dec and Jan.

This also removes the aoc subscribe command, since Python has a replacement for it now.

Did you:

@ChrisLovering ChrisLovering requested a review from ks129 as a code owner January 1, 2022 14:28
@ChrisLovering ChrisLovering added area: backend Related to internal functionality and utilities area: frontend Related to output and formatting category: events Related to events (AoC, Hacktoberfest) status: needs review Author is waiting for someone to review and approve type: enhancement Changes or improvements to existing features labels Jan 1, 2022
@onerandomusername
Copy link
Copy Markdown
Contributor

onerandomusername commented Jan 1, 2022

e7e0238

Dupe of #972

Actually, this isn't even a dupe as it doesn't use the already existing errors.

Copy link
Copy Markdown
Contributor

@onerandomusername onerandomusername left a comment

Choose a reason for hiding this comment

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

.aoc stats needs a restrictive decorator as well.

Might want to consider it for .aoc refresh as well, even though its staff only, it would cause a sentry issue.

@ChrisLovering
Copy link
Copy Markdown
Member Author

.aoc stats needs a restrictive decorator as well.

Might want to consider it for .aoc refresh as well, even though its staff only, it would cause a sentry issue.

Feel free to open an issue. Don't want to delay this PR though.

@onerandomusername
Copy link
Copy Markdown
Contributor

Feel free to open an issue. Don't want to delay this PR though.

Everything else works, this will just cause a sentry error in February as the code isn't made for that. (Tried it with month override to February.)

If I don't open an issue sentry will

Copy link
Copy Markdown
Contributor

@onerandomusername onerandomusername left a comment

Choose a reason for hiding this comment

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

looks good enough, few bugs, but as stated above, worth the risk.

@ChrisLovering
Copy link
Copy Markdown
Member Author

Feel free to open an issue. Don't want to delay this PR though.

Everything else works, this will just cause a sentry error in February as the code isn't made for that. (Tried it with month override to February.)

If I don't open an issue sentry will

"the code isn't made for that". Yes it is, you just need to set the AOC_YEAR env var, otherwise it will try to get next year's leaderboard.

Please don't make assumptions about the code and then say there are a "few bugs".

@onerandomusername
Copy link
Copy Markdown
Contributor

Yes it is, you just need to set the AOC_YEAR env var, otherwise it will try to get next year's leaderboard.

No where is it documented that this variable has to be set in production. The contributing guide says debug, which I presumed would not need to be used in prod.
image

link to guide

I now understand that it is currently being used in prod, but this was not made clear in the issue body, nor comments, until after my review.

@ChrisLovering
Copy link
Copy Markdown
Member Author

Yes it is, you just need to set the AOC_YEAR env var, otherwise it will try to get next year's leaderboard.

No where is it documented that this variable has to be set in production. The contributing guide says debug, which I presumed would not need to be used in prod. image

link to guide

I now understand that it is currently being used in prod, but this was not made clear in the issue body, nor comments, until after my review.

I can understand where you are coming from, really. The contribution guidelines can be made clearer in that respect, but there's only so much you can do with limited room in that table.

My point however was you are calling out this PR saying it has a few bugs. However, that area of the code hasn't even been changed in this PR, neither are they actual bugs.

While I don't mind it so much someone else, who is maybe new to contributing to our projects, would be put off by comments like this, so I wanted to raise it here so that it doesn't happen to someone else.

@onerandomusername
Copy link
Copy Markdown
Contributor

While I don't mind it so much someone else, who is maybe new to contributing to our projects, would be put off by comments like this, so I wanted to raise it here so that it doesn't happen to someone else.

yeah ofc

Copy link
Copy Markdown
Contributor

@janine9vn janine9vn left a comment

Choose a reason for hiding this comment

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

Nicely written code and works like a charm.

There is some consistency changes with the decorator month locks for this cog I'd like to see happen, but it doesn't have to be this PR and I'd rather get this PR merged asap. I'll make a separate PR to kaizen this cog a bit.

Copy link
Copy Markdown
Member

@Den4200 Den4200 left a comment

Choose a reason for hiding this comment

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

Splendid job! Works nicely.

@Den4200 Den4200 merged commit 175b7ef into main Jan 1, 2022
@Den4200 Den4200 deleted the aoc-changes branch January 1, 2022 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: backend Related to internal functionality and utilities area: frontend Related to output and formatting category: events Related to events (AoC, Hacktoberfest) status: needs review Author is waiting for someone to review and approve type: enhancement Changes or improvements to existing features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants