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

templates: add .email_domain() method to Signatures #4687

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

Conversation

ilyagr
Copy link
Collaborator

@ilyagr ilyagr commented Oct 22, 2024

This is slightly less pretty than making an Email type, but I think is much easier.

I would use instead of the last argument of:

  concat(
    label("email", signature.username()),
    "@",
    label("email",
      signature.email().remove_prefix(
        signature.username()
      ).remove_prefix("@")
    )
  )

(Aside for more motivation: I initially had a bug above by using .remove_prefix(signature.username() ++ "@"). This is wrong since an email might not contain an @, as I discovered while looking at the code. The bug is now fixed, but signature.email_domain() would avoid the problem)

The effect I was going for (though I'm still not sure whether and how many characters I want to trim to):

image

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • n/a I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@ilyagr ilyagr marked this pull request as ready for review October 22, 2024 01:56
@ilyagr ilyagr changed the title templates: add .email_domain() method to signatures TODO: test templates: add .email_domain() method to signatures Oct 22, 2024
@ilyagr ilyagr force-pushed the ig/email_domain branch 5 times, most recently from b76e95a to e284f9b Compare October 22, 2024 02:04
This is slightly less pretty than making an Email type,
but I think is much easier.

I would use instead of the last argument of:

```
  concat(
    label("email", signature.username()),
    "@",
    label("email",
      signature.email().remove_prefix(
        signature.username()
      ).remove_prefix("@")
    )
  )
```

(Aside for more motivation: I initially had a bug above by using
`.remove_prefix(signature.username() ++ "@")`. This is wrong since an
email might not contain an `@`, as I discovered while looking at the
code. The bug is now fixed, but `signature.email_domain()` would avoid
the problem)
@ilyagr ilyagr changed the title templates: add .email_domain() method to signatures templates: add .email_domain() method to Signatures Oct 22, 2024
@yuja
Copy link
Collaborator

yuja commented Oct 22, 2024

Maybe it's time to add an Email template type (and deprecate .username())? Suppose the goal is to label @ differently, an Email object could be rendered to <local>foo</local>@<domain>example.org</domain> by default.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Oct 22, 2024

That would be better, but also a lot more work. As I mentioned, this is giving me pause at the moment TBH. I haven't decided whether/when I want to do it.

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