feat: allow file uploads for cvar, uvar, svar, gvar and alias, servalias, snippet, servsnippet by 7he4lph4 · Pull Request #2207 · avrae/avrae

7he4lph4 · 2025-08-12T22:29:59Z

Summary

Extend gvar, alias, servalias, snippet, and servsnippet create/edit commands to accept file attachments, following the same pattern as cvar, uvar, and svar.

From PR #2206:

Read file attachments if no value is provided so that longer variables than a discord message can be set. Avrae already provides longer variables as a text file anyways, so this just allows full support in both directions.

~~Depends on PR #2206 - This PR builds on @LazyDope's cvar/uvar/svar file upload implementation. Please merge #2206 first to avoid conflicts.~~ (both PRs merged into this now)

Changelog Entry

!cvar, !uvar, !svar, !gvar, !alias, !servalias, !snippet, and !servsnippet now support uploading file attachments.

Checklist

PR Type

This PR is a code change that implements a feature request.
This PR fixes an issue.
This PR adds a new feature that is not an open feature request.
This PR is not a code change (e.g. documentation, README, ...)

Other

This PR is a breaking change (e.g. methods or parameters removed/renamed)
If code changes were made then they have been tested.
I have updated the documentation to reflect the changes.

LazyDope · 2025-08-12T22:49:27Z

I believe the changes to the read_file_from_message function will break viewing cvars, uvars, and svars.

LazyDope

if not ctx.message.attachments:
        raise IndexError("No attachments")

These lines are unnecessary, IndexError is already raised by ctx.message.attachments[0] in this scenario

7he4lph4 · 2025-08-13T00:43:23Z

if not ctx.message.attachments:
        raise IndexError("No attachments")
These lines are unnecessary, IndexError is already raised by ctx.message.attachments[0] in this scenario

Yea honestly, woulda removed that if I wasn't fixing it in a hurry, other than that, any other suggestions? I'll make this point to your fork?

Infinidoge · 2025-08-13T01:52:33Z

I'd personally keep the check. Raising the index error ourselves allows it to return a better error message than "Index out of bounds for attachments".

7he4lph4 · 2025-08-13T07:59:44Z

I'd personally keep the check. Raising the index error ourselves allows it to return a better error message than "Index out of bounds for attachments".

The IndexError happens naturally at ctx.message.attachments[0]like lazydope said and is being properly handled in _get_value_or_file()

LazyDope · 2025-08-13T16:28:36Z

I'd personally keep the check. Raising the index error ourselves allows it to return a better error message than "Index out of bounds for attachments".

We're not actually returning this error either way, it needs to be converted into an InvalidArgument error before being raised any further to avoid the error appearing as a bot error rather than an argument error.

I suppose explicitly raising the error could make it clearer to any other readers/users of the function that the IndexError is intetional and meant to caught.

7he4lph4 · 2025-08-14T04:39:03Z

I suppose explicitly raising the error could make it clearer to any other readers/users of the function that the IndexError is intetional and meant to caught.

@LazyDope Good point. Also, wrt catching all Exceptions in cvar/svar/uvar. Won't it be better to only catch IndexError there as well? And let other errors go through, coz say, the Discord CDN fails during a file upload, we won't be sending the "Error in Discord API. Try again" msg Avrae usually shows right? And all other sys errs go through as well.

LazyDope · 2025-08-14T21:26:08Z

I suppose explicitly raising the error could make it clearer to any other readers/users of the function that the IndexError is intetional and meant to caught.

@LazyDope Good point. Also, wrt catching all Exceptions in cvar/svar/uvar. Won't it be better to only catch IndexError there as well? And let other errors go through, coz say, the Discord CDN fails during a file upload, we won't be sending the "Error in Discord API. Try again" msg Avrae usually shows right? And all other sys errs go through as well.

My initial thought was if fetching the attachment failed then to just continue as if it didn't exist, however you're probably right that we should only catch IndexErrors

LazyDope · 2025-08-14T21:27:52Z

Also, what are your thoughts on merging the PRs into this one?

7he4lph4 · 2025-08-14T21:54:39Z

Also, what are your thoughts on merging the PRs into this one?

Sure yea, that works too

7he4lph4 · 2025-08-14T21:57:10Z

It's already technically merged tho lol, this PR contains all your commits too

LazyDope · 2025-08-14T23:48:28Z

It's already technically merged tho lol, this PR contains all your commits too

Just update the title and description to clarify that it covers both features.

- style: also added thousands separators to size limit err msgs

LazyDope · 2025-08-18T01:25:39Z

cogsmisc/customization.py

-            except InvalidArgument as e:
-                raise e
-            except Exception:
+            value = await _get_value_or_file(ctx, None, CVAR_FILE_SIZE_LIMIT, allow_empty=True)


With the addition of this helper function, we could probably just move this outside of the if value is None check, since that check is a bit redundant

Right, but the code readability is crystal clear here with that flow, making it self documenting. Removing that check would make it slightly obfuscated.

I'm open to removing tho if that tradeoff is fine.

I agree with @LazyDope. The name _get_value_or_file() is self documenting and cleans a level of nesting in the different places it's called

Sure, done. This was just me being overtly KISS at the cost of a little DRYness.

codybanman

This looks great and works well for the various cases I tested. As I mentioned in the thread, I agree with @LazyDope’s suggestion on the use of use _get_value_or_file(). Once that change is in, we can test it out on nightly.

codybanman · 2025-10-10T17:38:22Z

cogsmisc/customization.py

-            except InvalidArgument as e:
-                raise e
-            except Exception:
+            value = await _get_value_or_file(ctx, None, CVAR_FILE_SIZE_LIMIT, allow_empty=True)


I agree with @LazyDope. The name _get_value_or_file() is self documenting and cleans a level of nesting in the different places it's called

LazyDope added 3 commits August 8, 2025 13:41

feat: allow file uploads for cvar, uvar, and svar

5564c0d

docs: mention file upload in help text

348f832

fix: improve error for invalid upload

cba20b8

LazyDope suggested changes Aug 12, 2025

View reviewed changes

7he4lph4 force-pushed the cvar-file-upload branch from 54b75dd to 848d810 Compare August 13, 2025 00:55

feat: file upload support for gvar create, edit

4dca4dc

7he4lph4 force-pushed the cvar-file-upload branch from 848d810 to 4dca4dc Compare August 13, 2025 01:02

nit: less than -> not exceed (< vs <=)

7a2146c

LazyDope mentioned this pull request Aug 14, 2025

feat: allow file uploads for cvar, uvar, and svar #2206

Closed

7 tasks

refactor: make upload exception handling safe and DRY

d3b6f51

- style: also added thousands separators to size limit err msgs

7he4lph4 changed the title ~~feat: file upload support for gvar create, edit~~ feat: allow file uploads for cvar, uvar, svar, and gvar Aug 15, 2025

LazyDope reviewed Aug 18, 2025

View reviewed changes

test: add file upload tests for (c/u/s/g)vars

46c85a1

codybanman requested changes Oct 10, 2025

View reviewed changes

refactor: remove redundant check

6c3579a

7he4lph4 force-pushed the cvar-file-upload branch from fd5d15c to 5139e74 Compare October 16, 2025 19:56

7he4lph4 changed the title ~~feat: allow file uploads for cvar, uvar, svar, and gvar~~ feat: allow file uploads for cvar, uvar, svar, and gvar and alias, servalias, snippet, servsnippet Oct 16, 2025

7he4lph4 changed the title ~~feat: allow file uploads for cvar, uvar, svar, and gvar and alias, servalias, snippet, servsnippet~~ feat: allow file uploads for cvar, uvar, svar, gvar and alias, servalias, snippet, servsnippet Oct 17, 2025

feat: extend file uploads to alias/snippet

569b713

7he4lph4 force-pushed the cvar-file-upload branch from 5139e74 to 569b713 Compare October 17, 2025 07:24

codybanman merged commit b58381c into avrae:nightly Oct 28, 2025

Conversation

7he4lph4 commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changelog Entry

Checklist

PR Type

Other

Uh oh!

LazyDope commented Aug 12, 2025

Uh oh!

LazyDope left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

7he4lph4 commented Aug 13, 2025

Uh oh!

Infinidoge commented Aug 13, 2025

Uh oh!

7he4lph4 commented Aug 13, 2025

Uh oh!

LazyDope commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

7he4lph4 commented Aug 14, 2025

Uh oh!

LazyDope commented Aug 14, 2025

Uh oh!

LazyDope commented Aug 14, 2025

Uh oh!

7he4lph4 commented Aug 14, 2025

Uh oh!

7he4lph4 commented Aug 14, 2025

Uh oh!

LazyDope commented Aug 14, 2025

Uh oh!

LazyDope Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

7he4lph4 Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codybanman Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

7he4lph4 Oct 12, 2025

Choose a reason for hiding this comment

Uh oh!

codybanman left a comment

Choose a reason for hiding this comment

Uh oh!

codybanman Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

7he4lph4 commented Aug 12, 2025 •

edited

Loading

LazyDope left a comment •

edited

Loading

LazyDope commented Aug 13, 2025 •

edited

Loading

7he4lph4 Aug 18, 2025 •

edited

Loading