Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify expression alias logic #110

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

evanpurkhiser
Copy link
Contributor

Instead of using tuple indexing use a namedtuple mapped from the expression aliases. This makes readability / understanding of this logic a little easier.

ExpressionAlias = namedtuple('ExpressionAlias', 'regular, hashed')

# Non-standard expression aliases
EXPR_ALIASES = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also pulls this up into the module scope so we're not redeclaring this mapping every time

Copy link
Owner

Choose a reason for hiding this comment

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

moving the aliases top module level would be ok, but not in favor of a namedtuple because users shall be allowed to update easily the aliases.

Another problem is that this change is not at all retrocompatible. To allow retrocompat, maybe using a class attribute that dictcopy the module level one would be a solution.

Copy link
Owner

Choose a reason for hiding this comment

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

Damn, the above comment was not validated, so you didnt saw it... sorry :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

users shall be allowed to update easily the aliases.

What makes it difficult to update the aliases because we're using a named tuple? Could you share an example of what you're thinking that is more difficult?

Copy link
Owner

Choose a reason for hiding this comment

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

dict are mutable, not namedtuple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could just replace it like this though right

# Replace alias with whatever you want
EXPR_ALIASES['@midnight'] = ExpressionAlias("1 2 * * *", "h h * * *")

@evanpurkhiser
Copy link
Contributor Author

Updated to fix remove the conflict

@kiorky
Copy link
Owner

kiorky commented Oct 30, 2024

Updated to fix remove the conflict

oups, the other pr i just merged broke it again :( :) .

@evanpurkhiser
Copy link
Contributor Author

I'll get it fixed after I get the black formatting done :)

Instead of using tuple indexing use a namedtuple mapped from the
expression aliases. This makes readability / understanding of this logic
a little easier.
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