feat/improve configuration #75

onerandomusername · 2021-09-19T06:25:23Z

feat: rewrite the entire configuration system to use marshmallow and attrs
config: remove config_default.toml since its now part of the classes

Closes #22

codecov · 2021-09-19T07:24:22Z

Codecov Report

Merging #75 (f0ec067) into main (4b07917) will decrease coverage by 3.08%.
The diff coverage is 61.79%.

❗ Current head f0ec067 differs from pull request most recent head 4644d76. Consider uploading reports for the commit 4644d76 to get more accurate results

@@            Coverage Diff             @@
##             main      #75      +/-   ##
==========================================
- Coverage   70.90%   67.81%   -3.09%     
==========================================
  Files          33       35       +2     
  Lines        1619     2082     +463     
  Branches      199      286      +87     
==========================================
+ Hits         1148     1412     +264     
- Misses        439      610     +171     
- Partials       32       60      +28

Impacted Files	Coverage Δ
modmail/__main__.py	`0.00% <0.00%> (ø)`
modmail/extensions/meta.py	`73.91% <ø> (ø)`
modmail/extensions/extension_manager.py	`35.15% <22.22%> (+0.79%)`	⬆️
modmail/extensions/configuration_manager.py	`38.60% <38.60%> (ø)`
modmail/bot.py	`38.28% <50.00%> (-0.57%)`	⬇️
tests/modmail/conftest.py	`75.00% <57.14%> (-11.96%)`	⬇️
modmail/config.py	`67.13% <66.66%> (-29.79%)`	⬇️
tests/modmail/test_config.py	`95.94% <95.94%> (ø)`
modmail/__init__.py	`100.00% <100.00%> (ø)`
modmail/utils/embeds.py	`81.08% <100.00%> (-0.50%)`	⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d47e097...4644d76. Read the comment docs.

onerandomusername · 2021-09-27T02:14:29Z

This pr is now feature frozen, a full system to be able to change settings while the bot is running will be added later, the rudimentary system provided here is good enough for now.

Shivansh-007

It would be great to have some documentation on how to work with the config as a developer (adding, removing, meta), otherwise it works pretty well. I haven't tested out this via discord as I off it for a while. Secondly, I am not really familiar with attr (I should give it a shot, probably after exams).

.pre-commit-config.yaml

Shivansh-007 · 2021-09-30T23:45:36Z

pyproject.toml

+environs = "~=9.3.3"
+marshmallow = "~=3.13.0"
+python-dotenv = "^0.19.0"


Why do we have two deps which do the samething?

environs is being used here in order to set the logging level, #118 will make this unnecessary.

still stands

modmail/modmail/__init__.py

Lines 24 to 25 in 5afc196

ROOT_LOG_LEVEL = env.log_level("MODMAIL_LOG_LEVEL", logging.TRACE)

FMT = "%(asctime)s %(levelname)10s %(name)15s - [%(lineno)5d]: %(message)s"

Used here, but made irrelevant by #118 so I'm not fixing it here.

modmail/config.py

…attrs

Signed-off-by: onerandomusername <genericusername414+git@gmail.com>

…oken from being changed Signed-off-by: onerandomusername <genericusername414+git@gmail.com>

when converting to a marshmallow schema, desert does not use class defined Meta classes Signed-off-by: onerandomusername <genericusername414+git@gmail.com>

Signed-off-by: onerandomusername <genericusername414+git@gmail.com>

if any modmail configuration file is edited, a pre-commit hook will run to check that the autogenerated files are up-to-date Signed-off-by: onerandomusername <genericusername414+git@gmail.com>

tests: mark xfail the test for default config caching

…o app.json

… getters

…ndency

Shivansh-007

half reviewed, gtg so can't finish

.pre-commit-config.yaml

Shivansh-007 · 2021-12-04T16:18:28Z

modmail/config.py

+
+
+_log_optional_deps = logging.getLogger("modmail.optional_dependencies")
+_silence = " You can silence these alerts by ignoring the `modmail.optional_dependencies` logger"


Ignoring optional_dependencies where? Be a bit more concise, also am not sure if this is really highlighted since the user's won't be diving deep into the code.

I'm not even sure this logger is necessary, tbh

The only people who would silence this are developers, and I wonder if its even necessary. I might yank the warning entirely.

Shivansh-007 · 2021-12-04T16:19:00Z

modmail/config.py

+try:
+    import dotenv
+except ModuleNotFoundError:  # pragma: nocover
+    _log_optional_deps.notice("dotenv was unable to be imported." + _silence)


Not worth it since silence is always at the end.

Per this comment, I may be removing that warning anyways.

Shivansh-007 · 2021-12-04T16:26:20Z

modmail/config.py


+@functools.lru_cache
+def _get_config_directory() -> pathlib.Path:


Its also worth checking if the user supplied a --config arg in sys.argv. This would in cases when you are working on multiple scenarios and don't need to constantly keep changing the config while having the others in background.

Using --config you won't need to change them and can directly pass the config in.

Not really the write place, it would better fit in USER_CONFIG_FILES.

Might add it in a seperate pr, I don't want to add this here since it would involve adding a cli to the bot's entrypoint.

While I'm aware that's not hard, I don't want to reach much further out of the scope of this pr.

Shivansh-007 · 2021-12-04T16:30:03Z

modmail/config.py

+]
+
+
+class BetterPartialEmojiConverter(discord.ext.commands.converter.EmojiConverter):


could probably be moved to a file called patches.py since it likely to be used elsewhere in the code.

Shivansh-007 · 2021-12-04T16:56:39Z

modmail/extensions/configuration_manager.py

+        kw = {}
+        kw["default"] = field.default if field.default is not marshmallow.missing else None
+        kw["name"] = field.name
+        kw["type"] = field.type
+
+        kw["metadata"] = field.metadata
+        kw["field"] = field
+
+        kw["frozen"] = field.on_setattr is attr.setters.frozen or frozen
+
+        metadata_table: config.ConfigMetadata = field.metadata[config.METADATA_TABLE]
+        kw[config.METADATA_TABLE] = metadata_table
+        kw["description"] = metadata_table.description
+        kw["canonical_name"] = metadata_table.canonical_name
+        kw["extended_description"] = metadata_table.extended_description
+        kw["hidden"] = metadata_table.hidden
+
+        kw["discord_converter"] = metadata_table.discord_converter
+        kw["discord_converter_attribute"] = metadata_table.discord_converter_attribute
+
+        if nested is not None:
+            kw["nested"] = nested


Wouldn't it be more readable if we form the dict at once?

I don't think so, I felt like this was more readable.

I meant to add documentation to each key here in the form of comments, which I guess i didn't do.

Shivansh-007 · 2021-12-04T17:11:52Z

modmail/extensions/configuration_manager.py

+        await responses.send_positive_response(ctx, response)
+
+    @config_group.command(name="get", aliases=("show",))
+    async def get_config(self, ctx: Context, option: KeyConverter) -> None:


get shouldn't work if the value is hidden, like in the case of bot.token

Will fix when renovating the configuration manager in a new pr.

Shivansh-007 · 2021-12-04T17:17:49Z

modmail/utils/embeds.py

@@ -35,7 +33,7 @@ def __init__(self: discord.Embed, description: str = None, **kwargs):  # noqa: N
    if ("description" in kwargs or description is not None) and "content" in kwargs:
        raise TypeError("Description and content are aliases for the same field, but both were provided.")

-    colour = kwargs.pop("color", kwargs.pop("colour", DEFAULT_COLOR))
+    colour = kwargs.pop("color", kwargs.pop("colour", config().user.colours.base_embed_color))


bot is not available in this context.

Shivansh-007 · 2021-12-04T17:18:02Z

modmail/utils/extensions.py

@@ -34,8 +34,9 @@ def determine_bot_mode() -> int:
    The configuration system uses true/false values, so we need to turn them into an integer for bitwise.
    """
    bot_mode = 0
+    _config = config()


bot is not available in this context.

Shivansh-007 · 2021-12-04T17:18:19Z

pyproject.toml

+environs = "~=9.3.3"
+marshmallow = "~=3.13.0"
+python-dotenv = "^0.19.0"


still stands

restructuring, using 'startswith', other updates

coveralls · 2022-01-02T06:16:44Z

Coverage decreased (-2.8%) to 75.0% when pulling 4644d76 on feat/improve-configuration into d47e097 on main.

onerandomusername requested review from bast0006 and Shivansh-007 as code owners September 19, 2021 06:25

onerandomusername added p: high High Priority s: needs review Ready for review and merge t: enhancement Changes or improvements to existing features a: bot Catch all for issues regarding the bot labels Sep 19, 2021

onerandomusername force-pushed the feat/improve-configuration branch 6 times, most recently from fe7a268 to 0b511e3 Compare September 21, 2021 22:22

onerandomusername mentioned this pull request Sep 26, 2021

feat/error handler #77

Merged

onerandomusername mentioned this pull request Sep 27, 2021

enhancement: embedify all of the meta commands #78

Merged

onerandomusername force-pushed the feat/improve-configuration branch from b2bb705 to b5bd77a Compare September 29, 2021 19:50

Shivansh-007 reviewed Oct 1, 2021

View reviewed changes

onerandomusername added 12 commits October 4, 2021 01:03

feat: rewrite the entire configuration system to use marshmallow and …

b5c5417

…attrs

config: remove config_default.toml since its now part of the classes

6a1b0c2

Signed-off-by: onerandomusername <genericusername414+git@gmail.com>

fix: add missing items to the configuration

5afc196

Signed-off-by: onerandomusername <genericusername414+git@gmail.com>

fix: have a test token in the pytest env

f8fabf5

Signed-off-by: onerandomusername <genericusername414+git@gmail.com>

enable slots on cfg classes, fix colour converter, properly prevent t…

6cc4e20

…oken from being changed Signed-off-by: onerandomusername <genericusername414+git@gmail.com>

fix: remove unused class definitions

82b1c73

when converting to a marshmallow schema, desert does not use class defined Meta classes Signed-off-by: onerandomusername <genericusername414+git@gmail.com>

config: add autogenerated default cfg toml file

4a80406

Signed-off-by: onerandomusername <genericusername414+git@gmail.com>

fix: properly deserialize discord.Colour to a string representation

311ff54

Signed-off-by: onerandomusername <genericusername414+git@gmail.com>

feat: add configuration via a yaml cfg file

62aa7dc

Signed-off-by: onerandomusername <genericusername414+git@gmail.com>

fix: allow log level 50, as documented

e4d9e1e

Signed-off-by: onerandomusername <genericusername414+git@gmail.com>

feat: made testable config system by refactor config file loading

70128af

Signed-off-by: onerandomusername <genericusername414+git@gmail.com>

tools: add pre-commit hook to automatically export default config

4707d6c

if any modmail configuration file is edited, a pre-commit hook will run to check that the autogenerated files are up-to-date Signed-off-by: onerandomusername <genericusername414+git@gmail.com>

minor: use functools cache to cache the config

3d1986d

onerandomusername requested a review from bast0006 November 16, 2021 17:33

onerandomusername force-pushed the feat/improve-configuration branch from 4e38df2 to 62056c7 Compare November 16, 2021 17:46

onerandomusername added 3 commits November 16, 2021 12:47

use factories to generate mutable classes and add more logging

ff04a5a

tests: mark xfail the test for default config caching

fix: use config.user to return configured prefix

258fa1b

chore: export the extended description after the normal description t…

bfe641c

…o app.json

onerandomusername force-pushed the feat/improve-configuration branch from 62056c7 to bfe641c Compare November 16, 2021 17:47

onerandomusername added 3 commits November 21, 2021 10:30

chore: remove operator.attrgetter and implement recursive setters and…

fe53cf7

… getters

chore: make the error spam less spammy for missing yaml file and depe…

4e7fb22

…ndency

Merge branch 'main' into feat/improve-configuration

dd2f16c

onerandomusername force-pushed the feat/improve-configuration branch 3 times, most recently from c17131b to f015b56 Compare November 21, 2021 16:52

tests: fix environment not being replaced soon enough

03fd57f

onerandomusername force-pushed the feat/improve-configuration branch from f015b56 to 03fd57f Compare November 21, 2021 17:01

onerandomusername mentioned this pull request Nov 22, 2021

tests: fix environment not being replaced soon enough #126

Merged

fix: use type() instead of __class__

f0ec067

onerandomusername mentioned this pull request Nov 25, 2021

Permission system #130

Open

Shivansh-007 requested changes Dec 4, 2021

View reviewed changes

onerandomusername added 2 commits January 2, 2022 01:05

chore: address reviews

09a33d4

restructuring, using 'startswith', other updates

Merge branch 'main' into feat/improve-configuration

8d18ccc

Shivansh-007 mentioned this pull request Feb 2, 2022

Configuration system doublevcodes/sirius#4

Closed

onerandomusername added 5 commits April 20, 2022 21:33

remove dependency on environs

4087767

fix: pin discord.py

5911ff8

update black

768277e

update precommit hooks

62e82fa

add configuration system to docs

4644d76

onerandomusername merged commit 10a6468 into main Apr 21, 2022

onerandomusername deleted the feat/improve-configuration branch April 21, 2022 02:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat/improve configuration #75

feat/improve configuration #75

onerandomusername commented Sep 19, 2021 •

edited

codecov bot commented Sep 19, 2021 •

edited

onerandomusername commented Sep 27, 2021

Shivansh-007 left a comment

Shivansh-007 Sep 30, 2021

onerandomusername Dec 4, 2021

Shivansh-007 Dec 4, 2021

onerandomusername Jan 2, 2022

Shivansh-007 left a comment

Shivansh-007 Dec 4, 2021

onerandomusername Dec 4, 2021

onerandomusername Dec 15, 2021

Shivansh-007 Dec 4, 2021

onerandomusername Dec 15, 2021

Shivansh-007 Dec 4, 2021

onerandomusername Dec 15, 2021

Shivansh-007 Dec 4, 2021

Shivansh-007 Dec 4, 2021

onerandomusername Jan 2, 2022

Shivansh-007 Dec 4, 2021

onerandomusername Dec 15, 2021 •

edited

Shivansh-007 Dec 4, 2021

onerandomusername Jan 2, 2022

Shivansh-007 Dec 4, 2021

onerandomusername Jan 2, 2022

Shivansh-007 Dec 4, 2021

coveralls commented Jan 2, 2022 •

edited

	ROOT_LOG_LEVEL = env.log_level("MODMAIL_LOG_LEVEL", logging.TRACE)
	FMT = "%(asctime)s %(levelname)10s %(name)15s - [%(lineno)5d]: %(message)s"



		_log_optional_deps = logging.getLogger("modmail.optional_dependencies")
		_silence = " You can silence these alerts by ignoring the `modmail.optional_dependencies` logger"


		@functools.lru_cache
		def _get_config_directory() -> pathlib.Path:

		]


		class BetterPartialEmojiConverter(discord.ext.commands.converter.EmojiConverter):

feat/improve configuration #75

feat/improve configuration #75

Conversation

onerandomusername commented Sep 19, 2021 • edited

codecov bot commented Sep 19, 2021 • edited

Codecov Report

onerandomusername commented Sep 27, 2021

Shivansh-007 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shivansh-007 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onerandomusername Dec 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 2, 2022 • edited

onerandomusername commented Sep 19, 2021 •

edited

codecov bot commented Sep 19, 2021 •

edited

onerandomusername Dec 15, 2021 •

edited

coveralls commented Jan 2, 2022 •

edited