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

Make the Watchdog#restart() method private #14

Merged
merged 1 commit into from
Jul 22, 2019
Merged

Make the Watchdog#restart() method private #14

merged 1 commit into from
Jul 22, 2019

Conversation

ma2ciek
Copy link
Contributor

@ma2ciek ma2ciek commented Jul 22, 2019

Suggested merge commit message (convention)

Other: Made the Watchdog#restart() method private. Changed the signatures of Watchdog#create() and Watchdog#destroy(), so now these methods will return empty promises. Closes ckeditor/ckeditor5#4701.

@ma2ciek ma2ciek requested review from pjasiun and Reinmar July 22, 2019 10:01
Copy link
Member

@Reinmar Reinmar left a comment

Choose a reason for hiding this comment

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

The PR description says that you've made restart() private so I expected to see a simple change in the docs and an underscore being added here and there. The proposed changes are much bigger though, so I'd like to understand why – was the scope of this ticket increased or I don't understand it.

@ma2ciek
Copy link
Contributor Author

ma2ciek commented Jul 22, 2019

These are the only things I made within this PR:

  • I moved the restart() method to the bottom and I renamed it to _restart().
  • I stopped using the _restart() method in tests (as it became private), now I throw an error to perform the restart action instead.
  • Since the watchdog is accessible after creation and destruction, there's no need to use watchdog.create().then( watchdog => {} ) and watchdog.destroy().then( watchdog => {} ), so I simplified these implementations and made these methods return empty promises. (This was rather a cosmetical change, I don't think anyone would use this feature).

@ma2ciek ma2ciek requested a review from Reinmar July 22, 2019 14:18
@Reinmar Reinmar merged commit 69aef8b into master Jul 22, 2019
@Reinmar Reinmar deleted the t/13 branch July 22, 2019 14:39
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.

Watchdog#restart() should not be public
2 participants