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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix broken doc errors #12900

Merged
merged 7 commits into from May 11, 2018

Conversation

Projects
None yet
4 participants
@ckerr
Copy link
Member

ckerr commented May 11, 2018

Fix a handful of errors that prevented create-typescript-definitions from running in master.

  • Fix missing documentation for methods mentioned in 'breaking-changes.md' that caused the run to error out with "error TS2339"
  • Fix routingId type. Previous phrasing confused the parser. Previously, the string "A unique id" got parsed as routingId being of type unique 馃槂
  • Tweak Referrer.policy type so that all the possible values were read. Previously, the parser got confused by the word 'or' in 'Can be foo, bar, or mum'.
  • Make LoadURLOptions.Referrer type consistent in the different files. Some definitions had it as a string; others had it as a string | Referrer, which caused the run to fail with Error: Interface "LoadURLOptions" has already been declared (cc @jkleinsc)

ckerr added some commits May 11, 2018

Fix routingId type
Previous phrasing confused electron-typescript-definitions,
thinking that routingId was of type `unique` 馃槂
Reinstate docs for app.makeSingleInstance()
Since this method is mentioned in breaking-changes.md, we need
the docs to prevent create-typescript-definitions from failing with
"error TS2339: Property 'makeSingleInstance' does not exist on type 'App'."

The reinstated docs are marked with deprecation warnings
that refer the reader to the new API calls.

@ckerr ckerr requested review from jkleinsc and MarshallOfSound May 11, 2018

@ckerr ckerr requested review from as code owners May 11, 2018

@ckerr ckerr changed the title fix broken documentation fix broken doc errors May 11, 2018

@@ -10,6 +10,7 @@
"chai-as-promised": "^7.1.1",
"coffee-script": "1.12.7",
"dbus-native": "^0.2.3",
"fs-extra": "^6.0.0",

This comment has been minimized.

@zeke

zeke May 11, 2018

Member

Is this needed?

This comment has been minimized.

@ckerr

ckerr May 11, 2018

Author Member

Gah, no that's from an unrelated task that shouldn't've been in this PR. Good catch; will remove

@jkleinsc
Copy link
Contributor

jkleinsc left a comment

Looks good to me. Thanks for the PR @ckerr

@jkleinsc jkleinsc merged commit bbba9ff into master May 11, 2018

10 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-osx-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@zeke zeke deleted the make-httpReferrer-docs-consistent branch May 11, 2018

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented May 12, 2018

Will do a follow up PR for this into electron-typescript-definitions we don't want to document API's that are deprecated as they will still show up in the defintions. The best way to force people off the API's is to completely remove them from the docs and therefore from the definitions (it's what we've done in the past)

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented May 12, 2018

Actually there is already a PR that fixes this --> electron/typescript-definitions#101

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.