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

SlackReporter with preformatted rich text #780

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vimagick
Copy link

@vimagick vimagick commented Dec 5, 2023

Hi, I've updated my code ( #776 )

Three things:

  • Have you checked whether or not Mattermost supports Slack's rich text feature?

     Mattermost still use the original `{"text": text}`
    
  • How about users who want to opt out of this new behavior and have the old (non-rich-text) behavior? Should there be an option? What should be the default?

    Enabled only when `rich_text: true`
    
  • Please update CHANGELOG.md with information about this change

    Yes
    

Copy link
Owner

@thp thp left a comment

Choose a reason for hiding this comment

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

You don't need to create a new PR when you update the PR. In fact, it makes reviewing a little more difficult. You can just update the PR. If you want to rewrite Git history, you can force-push to your remote branch and the PR will be updated.

Some more comments:

  • In the CHANGELOG.md, feel free to link the PR + add your credits
  • Can you add 'rich_text': False, to lib/urlwatch/storage.py in the 'slack': section, so that it's easily discoverable?
  • Can you add documentation for this to the Slack section in docs/source/reporters.rst

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Thomas Perl <m@thp.io>
@thp
Copy link
Owner

thp commented Mar 12, 2024

@vimagick Did you have time to look into these?

  • Can you add 'rich_text': False, to lib/urlwatch/storage.py in the 'slack': section, so that it's easily discoverable?
  • Can you add documentation for this to the Slack section in docs/source/reporters.rst

@thp
Copy link
Owner

thp commented Apr 24, 2024

@vimagick Needs rebasing, and there's still the two items from above. Thanks!

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.

None yet

2 participants