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

Improving the installation docs and config.sample.yaml #389

Merged
merged 20 commits into from
Oct 1, 2020

Conversation

jaller94
Copy link
Contributor

@jaller94 jaller94 commented Mar 31, 2020

  • In the docs, be explicit which config fields need to be changed.
  • Simplify config.sample.yaml
    • Move homeserver, db and matrix_admin_room to the top. They will likely get changed by an admin.
    • Comment out tls and oauth2. They are optional, and tls prevents the server from starting in the former state.
  • Rename the page "Workspace Sync" to the more common name "Team Sync"

Christian Paul added 4 commits April 1, 2020 00:01
* Move `homeserver`, `db` and `matrix_admin_room` to the top. They will likely get changed by an admin.
* Comment `tls` and `oauth2`. They are optional, and `tls` prevents the server from starting in the former state.
@jaller94 jaller94 changed the title Small corrections to the docs Improving the installation docs and config.sample.yaml Mar 31, 2020
@jaller94 jaller94 requested review from Half-Shot and a team and removed request for Half-Shot April 1, 2020 10:54
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

A mostly stylistic review from someone unfamiliar with the project. Will likely need another review from someone about the correctness of everything.

README.md Show resolved Hide resolved
changelog.d/389.doc Outdated Show resolved Hide resolved
config/config.sample.yaml Outdated Show resolved Hide resolved
config/config.sample.yaml Outdated Show resolved Hide resolved
# Optional, will use `url` by default.
# media_url: "http://my.server.here"
# Optional. The maximum size of a uploaded file to Matrix in bytes. No limit by default
# max_upload_size: 104857600
Copy link
Member

Choose a reason for hiding this comment

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

Could you either:

  • Remove the space before commented out option names or
  • Put a newline between options

as I find it hard to notice that there are multiple options in this comment blob.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit easier with syntax highlighting.
All my code editors put a space after the comment character, so it's very annoying to maintain comments without the space.
Adding newlines makes the file length significantly longer and less readable when introducing a newline consistently between all properties.

Copy link
Member

Choose a reason for hiding this comment

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

I disagree that:

  # The domain name of your homeserver
  server_name: my.server.here

  # The URL which the bridge server can reach the homeserver on.
  # You can use localhost so long as the bridge and homeserver are
  # hosted on the same machine
  # 8008 is assumed to be the port the homeserver listens on
  url: http://localhost:8008

  # The public facing url for media on your homeserver.
  # This is usually the public url of your homeserver.
  # Optional, will use `url` by default.
  # media_url: "http://my.server.here"

  # Optional. The maximum size of a uploaded file to Matrix in bytes. No limit by default
  # max_upload_size: 104857600

  # Optional. Used to specify the port of the appservice in the config, rather than the cli.
  # If this is defined, it will **override** the port given in the process arguments.
  # appservice_port: 9999

is less readable than:

  # The domain name of your homeserver
  server_name: my.server.here
  # The URL which the bridge server can reach the homeserver on.
  # You can use localhost so long as the bridge and homeserver are
  # hosted on the same machine
  # 8008 is assumed to be the port the homeserver listens on
  url: http://localhost:8008
  # The public facing url for media on your homeserver.
  # This is usually the public url of your homeserver.
  # Optional, will use `url` by default.
  # media_url: "http://my.server.here"
  # Optional. The maximum size of a uploaded file to Matrix in bytes. No limit by default
  # max_upload_size: 104857600
  # Optional. Used to specify the port of the appservice in the config, rather than the cli.
  # If this is defined, it will **override** the port given in the process arguments.
  # appservice_port: 9999

All my code editors put a space after the comment character, so it's very annoying to maintain comments without the space.

I have to ask how often you add new options to this config file for hitting backspace once to be a major inconvenience.

Copy link
Member

Choose a reason for hiding this comment

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

These are just suggestions that we apply to synapse's config, which while monumentally longer, we don't receive complaints about the length.

If you don't want to apply them though, that's fine - it's only a suggestion.

config/config.sample.yaml Outdated Show resolved Hide resolved
config/config.sample.yaml Outdated Show resolved Hide resolved
config/config.sample.yaml Outdated Show resolved Hide resolved
config/config.sample.yaml Outdated Show resolved Hide resolved
config/config.sample.yaml Outdated Show resolved Hide resolved
Christian Paul and others added 7 commits June 15, 2020 11:40
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

Looks good other than the readability responses I left above.

@@ -153,7 +153,7 @@ export class Main {
}

let bridgeStores = {};
const usingNeDB = config.db === undefined;
const usingNeDB = config.db === undefined || config.db?.engine === "nedb";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Half-Shot Explicitly setting config.db.engine to "nedb" never worked, did it?

Also, do you agree with this change to format the sample file like Synapses sample with a lot more spacing for comments? Anoa suggested it and I like it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, do you agree with this change to format the sample file like Synapses sample with a lot more spacing for comments? Anoa suggested it and I like it.

Spacing never hurt anyone, I'm all for it.

Explicitly setting config.db.engine to "nedb" never worked, did it?

It should do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comments in the sample config and the schema list "nedb" as a valid value.

Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

I'm generally happy that this is an improvement, thank you for taking this on!


# Optional. Set the avatar.
#
avatar_url: "mxc://half-shot.uk/ea64c71ee946ca2f61379abefe2c7d977d276fbb"
Copy link
Contributor

Choose a reason for hiding this comment

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

We've since added new options to the config, can you double check we have all of them included in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked again that all values are also present in the new file. Some optional values have been commented out.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see anything in there for the encryption stuff?

@jaller94 jaller94 merged commit 3483321 into develop Oct 1, 2020
@jaller94 jaller94 deleted the jaller94-patch-1 branch October 2, 2020 08:59
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

3 participants