Skip to content

Conversation

@sezanzeb
Copy link
Owner

@sezanzeb sezanzeb commented Nov 10, 2024

  • The available arguments of macro functions (now called Tasks) are now very clearly defined, instead of using function-inspection and parameter-name conversions to do this stuff
  • Each task has its own class. They can be extended in there without cluttering the Macro object anymore. There is space for more complex macros
  • Task classes are very easy to understand now I think. New macros are easier to write.
  • It uses the same class for both constant (KEY_A and stuff) and variable arguments ($foo), instead of wrapping the latter, while leaving the first one raw.

The new architecture is certainly bulkier than the old one with all the new classes, but I think it's worth it. New classes are: Task, Argument, ArgumentConfig, RawValue. Variable has been extended.

Renamed MacroParsingError to MacroError because this stuff is also flying around during runtime.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

mypy

[mypy] reported by reviewdog 🐶
error: Argument 3 to "Task" has incompatible type "Context | None"; expected "Context" [arg-type]

@sezanzeb sezanzeb force-pushed the macro_refactor branch 8 times, most recently from 3a3d95a to e251fcd Compare November 10, 2024 14:54
@sezanzeb sezanzeb requested a review from jonasBoss November 10, 2024 21:36
@sezanzeb sezanzeb changed the title Refactor macro system Better macro architecture Nov 10, 2024
@sezanzeb sezanzeb marked this pull request as ready for review November 13, 2024 20:19
@sezanzeb
Copy link
Owner Author

@jonasBoss no hard feelings if you are busy with other stuff. Maybe I could just merge this and the other PRs (#1004 and #1007) slowly one by one, and maybe in the future, if you feel like it, make a review and I'll fix any comments in a new PR.

@sezanzeb sezanzeb merged commit 291b982 into main Dec 1, 2024
6 checks passed
@sezanzeb sezanzeb mentioned this pull request Dec 16, 2024
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.

2 participants