Skip to content

Conversation

@justindirose
Copy link
Contributor

Purely cosmetic PR. After internal discussion, the team decided to rename this plugin to discourse-docs.

A few implications that may need sorted:

  • Site settings do not get migrated over because we've renamed them. I think this could be handled by a migration to copy the rows over.
  • I currently have the imports of the Docs Ember model following the path of discourse-docs, but this will require the repository is renamed as well, which has other implications I believe.

@ZogStriP
Copy link
Member

  • Site settings do not get migrated over because we've renamed them. I think this could be handled by a migration to copy the rows over.

Yup. There probably are examples in core.

@justindirose
Copy link
Contributor Author

Added a settings migration today, so that should be okay to go. I believe front-end tests are failing here because of the repo name difference.

Copy link

@eviltrout eviltrout left a comment

Choose a reason for hiding this comment

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

The change itself seems sound.

One thing we should consider is this is a big breaking change. If any plugins or themes are built on top of the plugin they will likely break. We should be careful merging if any customers/big users of discourse will be affected.

@justindirose
Copy link
Contributor Author

One thing we should consider is this is a big breaking change. If any plugins or themes are built on top of the plugin they will likely break. We should be careful merging if any customers/big users of discourse will be affected.

Fully agree. I'll do some digging but will start by posting to Meta about this change to give a heads up.

@justindirose justindirose merged commit f32aebd into master Jan 18, 2021
@justindirose justindirose deleted the rename branch January 18, 2021 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants