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.

Sign up for GitHub

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

Jump to bottom

[BUGFIX] missing modules docs for tryInvoke, compare, isEqual #16079 #16080

Merged
merged 1 commit into from
Jan 8, 2018
Merged

[BUGFIX] missing modules docs for tryInvoke, compare, isEqual #16079 #16080

merged 1 commit into from
Jan 8, 2018

Conversation

jenweber
Copy link
Contributor

@jenweber jenweber commented Jan 6, 2018

See #16079. This should target at least 2.18 LTS candidate and 3.0-beta. 2.16 LTS would be good too. I have no idea how any of that is done so let me know if I need to open more PRs against other branches.

Needs a good look by @toddjordan

Thank you to sukima for reporting!

@jenweber jenweber mentioned this pull request Jan 6, 2018
Closed
@jenweber
Copy link
Contributor Author

jenweber commented Jan 6, 2018

The test suite passes locally.

CI fails at yarn install.
error An unexpected error occurred: "https://registry.yarnpkg.com/pinkie-promise/-/pinkie-promise-2.0.1.tgz: Request failed \"404 Not Found\"".

@jenweber
Copy link
Contributor Author

jenweber commented Jan 7, 2018 •
edited
Loading

I added missing compare and isEqual which should belong to @ember/utils

docs here

@jenweber jenweber changed the title [BUGFIX] missing modules docs for tryInvoke and canInvoke #16079 WIP [BUGFIX] missing modules docs for tryInvoke, canInvoke, compare #16079 Jan 7, 2018
@jenweber jenweber changed the title WIP [BUGFIX] missing modules docs for tryInvoke, canInvoke, compare #16079 WIP [BUGFIX] missing modules docs for tryInvoke, canInvoke, compare, isEqual #16079 Jan 7, 2018
@jenweber jenweber changed the title WIP [BUGFIX] missing modules docs for tryInvoke, canInvoke, compare, isEqual #16079 [BUGFIX] missing modules docs for tryInvoke, canInvoke, compare, isEqual #16079 Jan 7, 2018
@jenweber jenweber mentioned this pull request Jan 7, 2018
Closed
toddjordan
toddjordan reviewed Jan 8, 2018
View reviewed changes
packages/ember-runtime/lib/is-equal.js
@@ -30,7 +33,7 @@
```

@method isEqual
Copy link
Contributor

Choose a reason for hiding this comment

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

Also needs an @static to show up as a function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

toddjordan
toddjordan reviewed Jan 8, 2018
View reviewed changes
packages/ember-runtime/lib/compare.js
@@ -72,7 +76,7 @@ function spaceship(a, b) {
```

@method compare
Copy link
Contributor

Choose a reason for hiding this comment

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

needs an @static

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

toddjordan
toddjordan reviewed Jan 8, 2018
View reviewed changes
packages/ember-utils/lib/invoke.js Outdated
@@ -12,7 +15,7 @@ import applyStr from './apply-str';
```

@method canInvoke
Copy link
Contributor

Choose a reason for hiding this comment

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

@static

Copy link
Contributor Author

@jenweber jenweber Jan 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I removed my changes to canInvoke - turns out it's not a package/module which makes sense because it is private API

toddjordan
toddjordan reviewed Jan 8, 2018
View reviewed changes
packages/ember-utils/lib/invoke.js
@@ -35,7 +38,7 @@ export function canInvoke(obj, methodName) {
```

@method tryInvoke
Copy link
Contributor

Choose a reason for hiding this comment

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

@static

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

@jenweber
Copy link
Contributor Author

jenweber commented Jan 8, 2018

Thanks, I'll get those changes made

@jenweber jenweber changed the title [BUGFIX] missing modules docs for tryInvoke, canInvoke, compare, isEqual #16079 [BUGFIX] missing modules docs for tryInvoke, compare, isEqual #16079 Jan 8, 2018
@jenweber
Copy link
Contributor Author

jenweber commented Jan 8, 2018

@toddjordan I added static to everything and double-checked that these methods are actually available in the utils module/package. canInvoke wasn't, so I reverted those changes.

@toddjordan
Copy link
Contributor

Tried it out. Looks good! Thanks @jenweber !!!

@toddjordan toddjordan merged commit db34024 into emberjs:master Jan 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toddjordan toddjordan toddjordan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jenweber @toddjordan