Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add default values for notif_template_html and notif_template_text #7123

Closed
wants to merge 3 commits into from

Conversation

kuba-orlik
Copy link

@kuba-orlik kuba-orlik commented Mar 21, 2020

Ref #6551

When setting up synapse I found the lack of default values for these fields confusing. I encountered #6551 and decided to add the fields to sample config. Is it where defaults are taken from? If not, please correct me

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
  • Pull request includes a sign off
  • Code style is correct (run the linters)

Ref matrix-org#6994 

When setting up synapse I found the lack of default values for these fields confusing. I encountered matrix-org#6994 and decided to add the fields to sample config. Is it where defaults are taken from? If not, please correct me
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, but:

all in all... I'm a bit confused by what you are trying to achieve here.

@kuba-orlik
Copy link
Author

* this doesn't seem related to #6994?

Ow, that's right. I think I mis-clicked the issue number suggestion that GH tried to autocomplete. I meant issue #6551. I've updated the description. Sorry for the mess.

* there should be no need to set these options; they are deprecated and were recently removed from the sample doc.

I set out to create this PR when I was setting up Synapse and it threw an error because these options were not define, as mentioned in #6551

* the checklist in the PR template is meant to be followed.

I will follow up on them as soon as I free some time to work on that further. I hope that I cleared up my intentions with this PR here :)

@deepbluev7
Copy link
Contributor

I set out to create this PR when I was setting up Synapse and it threw an error because these options were not define, as mentioned in #6551

Please verify, if that still happens in 1.12.0. As far as I'm aware, this isn't an issue anymore.

@richvdh
Copy link
Member

richvdh commented Mar 26, 2020

yeah I think this is misguided. closing for now at least.

@richvdh richvdh closed this Mar 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants