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

docs: Refactor documented functions to be methods rather than properties #2127

Closed
wants to merge 4 commits into from
Closed

Conversation

DarhkVoyd
Copy link

Description

This PR aims to close the issue for methods to not be rendered as properties on the API docs.
Closes: #2096

Notes & open questions

The issue was label good first issue, since I am new to this project and unfamiliar with the codebase, I wanted review that if I am on the correct path cause the linting issue don't seem to decrease instead increase with more complex understanding of the codebase.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

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

Many of these changes shouldn't be necessary since aegir v41.0.0 only introduced the enforcement of method signature style rather than property function style.

Please revert all non-package.json changes, then run npm run clean at the root, then npm i and then npm run lint- you should only see errors like this which will indicate that you need to refactor some functions signatures.

as @wemeetagain mentioned running npm run lint -- -- --fix at the root should resolve all / most of these.

@@ -285,7 +285,7 @@ class CircuitRelayTransport implements Transport {
})

log('new outbound transient connection %a', maConn.remoteAddr)
return await this.upgrader.upgradeOutbound(maConn, {
return this.upgrader.upgradeOutbound(maConn, {
Copy link
Member

@achingbrain achingbrain Oct 6, 2023

Choose a reason for hiding this comment

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

Was this flagged by the linter? This sort of return style will introduce bugs:

try {
  return await fn()
} catch (err) {
  // if fn() rejects it will be caught here
}

vs:

try {
  return fn()
} catch (err) {
  // if fn() rejects it will NOT be caught here
}

@achingbrain
Copy link
Member

@DarhkVoyd I'm really sorry - #2096 should not have been tagged as a good first issue. As you've found it's results in an awful lot of changes to the codebase, some of which have non-obvious knock-on effects.

@DarhkVoyd
Copy link
Author

@achingbrain Hey there, so what do you suggest I do now?

@maschad
Copy link
Member

maschad commented Oct 6, 2023

@DarhkVoyd it's unfortunate that these issues came up, I am not sure why as running npm run lint -- -- --fix didn't resolve the issues on your branch as it did on #2132 , regardless #2137 has been merged.

@maschad maschad closed this Oct 6, 2023
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.

docs: Refactor documented functions to be methods rather than properties
3 participants