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

Move markdown to slack module #64

Closed
nobe4 opened this issue Apr 9, 2024 · 8 comments
Closed

Move markdown to slack module #64

nobe4 opened this issue Apr 9, 2024 · 8 comments

Comments

@nobe4
Copy link
Contributor

nobe4 commented Apr 9, 2024

I think that https://github.com/rneatherway/gh-slack/tree/main/internal/markdown would be better suited to live in https://github.com/rneatherway/slack and maybe having a markdown => slack-markdown function as well.

This would allow other clients to format markdown based on the code you've already written :)

What do you think?

If that's acceptable I'll PR that :)

@rneatherway
Copy link
Owner

I think that https://github.com/rneatherway/gh-slack/tree/main/internal/markdown would be better suited to live in https://github.com/rneatherway/slack and maybe having a markdown => slack-markdown function as well.

This would allow other clients to format markdown based on the code you've already written :)

What do you think?

Sure, that seems to make sense. I thought it might be a bit awkward because of the username cache but it looks like there is already an interface covering that part.

I'm not quite sure what you mean by markdown => slack-markdown but if you think it's useful then it probably is and the PR will explain it to me 😉

@nobe4
Copy link
Contributor Author

nobe4 commented Apr 10, 2024

I'm not quite sure what you mean by markdown => slack-markdown

Currently your code does slack-markdown => markdown, so I am thinking that a function that does the reverse would be useful as well.

I only wished slack didn't have its own version of markdown, feels unnecessarily convoluted.

@nobe4
Copy link
Contributor Author

nobe4 commented Apr 10, 2024

Here you go:

This does only the slack-markdown => markdown for now, since it might not be accepted. But if it is, I might spend some extra time to do the reverse :)

@rneatherway
Copy link
Owner

Both PRs LGTM, feel free to merge when ready.

As an FYI, a couple of thoughts that I had that are relevant here:

  1. I imagined that at some point it would make sense to move to the "rich" version of the Slack messages. By which I mean if you look at https://api.slack.com/surfaces/messages#payloads we are currently using text and it might be easier to give a consistently good result by instead reading blocks.
  2. I would like to keep the slack package at 0.x for a while and reserve the right to make breaking changes to it as I'm far from confident that the split is in the right place or that certain APIs are the best they can be. I expect that only this repo and possibly whatever you are using it for are the only consumers so far though so that isn't likely to be an issue!

@nobe4
Copy link
Contributor Author

nobe4 commented Apr 12, 2024

Both PRs LGTM, feel free to merge when ready.

🙏 🎉

  1. Agreed, the text is very limited and whenever I try to send 'fancy' messages, I always use attachments/blocks. However, this might already be possible via slackclient.API ?
  2. Definitely, I am working on another client atm (will share more when it's doing something good) and happy to adapt my code if the slack package changes :)

@rneatherway
Copy link
Owner

  • Agreed, the text is very limited and whenever I try to send 'fancy' messages, I always use attachments/blocks. However, this might already be possible via slackclient.API ?

I was thinking more of reading rather than writing. That is, when downloading Slack messages to convert them to github-flavoured markdown use the blocks as input rather than text.

@rneatherway
Copy link
Owner

This does only the slack-markdown => markdown for now, since it might not be accepted. But if it is, I might spend some extra time to do the reverse :)

Perfectly happy to accept the inverse function as well if you'd like to add that.

@nobe4
Copy link
Contributor Author

nobe4 commented May 3, 2024

For now I'll not do that, so I'll close the ticket :)

@nobe4 nobe4 closed this as completed May 3, 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

No branches or pull requests

2 participants