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

chore: Move jsdocs to interfaces #1356

Merged
merged 3 commits into from
Aug 1, 2023
Merged

Conversation

spalladino
Copy link
Collaborator

@spalladino spalladino commented Aug 1, 2023

Our current eslint setup was enforcing all public methods in classes to have a jsdoc, but not interfaces. As a result, most of our interfaces (where we want to have docs the most) were undocumented, and the classes that implement them carried the documentation instead.

This PR changes the eslint config so we enforce all interface methods to have documentation. All classes that implement them automatically inherit their docs (in vscode intellisense and in typedoc), so we don't need to repeat the docs there and can clean those up. Unfortunately, require-jsdoc cannot detect this automatically (see gajus/eslint-plugin-jsdoc#768), so we tweak the config so that jsdoc is not required for methods of classes that implement an interface (not perfect, but good enough). This new esquery filter required bumping the version of tslint.

Another issue with our config is that methods that are public by default (ie without an accessibility modifier) do not have jsdoc enforced. I tried enabling that but got 148 missing docs - so I'll leave that out for now, and is commented in the eslintrc file.

@spalladino spalladino force-pushed the palla/jsdocs-in-interfaces branch from 0c3abb0 to 64fcda1 Compare August 1, 2023 18:59
@spalladino spalladino requested a review from ludamad August 1, 2023 18:59
@ludamad
Copy link
Collaborator

ludamad commented Aug 1, 2023

Related to hitting edge cases, some ideas here #1154 about how we might just say eff it writing a custom eslint plugin isn't too bad :P

Copy link
Collaborator

@ludamad ludamad left a comment

Choose a reason for hiding this comment

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

Good with this

@ludamad
Copy link
Collaborator

ludamad commented Aug 1, 2023

For hard migrations (i.e. the 148 doc issue), we can do the pattern of having a copied eslint file, and doing it module by module

@spalladino spalladino enabled auto-merge (squash) August 1, 2023 19:17
@spalladino spalladino merged commit 7f7519d into master Aug 1, 2023
@spalladino spalladino deleted the palla/jsdocs-in-interfaces branch August 1, 2023 19:18
codygunton pushed a commit that referenced this pull request Jan 23, 2024
* Audit preperation (#1305)

* compiler warnings

* add solhint + fix small linter issues

* remove skipped tests

* update rollup address

* add underscores on arguments in IVerifier

* JB/Move global error to toast group (#1251)

* Move global error message inside toast group

* Mini refactor

* Mini refactor

* Add deferrals

* Blockchain test jest -> mocha update (#1314)

* run jest-to-mocha script

* Manually fix the last test + add coverage + formatting

* extend mocha timeout + bump optimizer runs

* decrease optimizer runs to deploy old rollup processor in testing

* remove coverage package, install on demand

* make solidity-coverage import a comment in hardhat config

* JB/Fix settlement times (#1315)

Fix batched time, fix button regression

* JB/Allow for debug to be exportable (#1313)

* Allow for debug to be exportable

* Remove exportable debug

* Rollback

* Refactor

* Tweak export logs

* Upgrade to latest bridge clients (#1319)

- Align with interface on latest bridge-clients
- Correct stETH address
- Calc element ROI via getTermAPR
- Calc stETH via getUnderlyingAsset

* Explain when to squash vs merge (#1302)

* Zw/update aztec connect specs (#1048)

* updated account circuit

* updated account circuit formatting

* updated schnorr spec

* updated intro

* fix typo

* updates in line with PR comments and rebase

Co-authored-by: Josh <[email protected]>
Co-authored-by: iAmMichaelConnor <[email protected]>

* Implemented rate limiting of deposits and address blacklist (#1323)

* Implemented rate limiting of deposits and address blacklist

* Cleanup

* Additionally check num deposits in tx receiver (#1326)

* Additionally check num deposits in tx receiver

* Sanity check input

* Additional tests (#1327)

* JB/Connect yearn bridge (#1334)

* Add yearn recipe

* Connect yearn bridge

* Fix toast z-index

* Fix price estimation for yearn bridge

* Take out console log

* Set up yvDAI bridge config

* Add styling for yearn (wip)

* Add price fetcher for yvDAI and yvETH

* Fix icon

* Fix exit modal

* Fixes and improvements

* Add collapse button to Toast component (#1322)

* don't expose blacklist (#1352)

* don't expose blacklist

* don't use delete keyword

* fix status endpoint types

* format

* fix server

* fix server

* Expose blacklist as undefined from public api.

Co-authored-by: Leila Wang <[email protected]>

* Jcf/hotfix yearn exit (#1356)

* - allow exiting via a different bridgeAddressId
- fix incorrect batch info shown for exiting
- more explicit name: useDefaultEnterBridgeCallData
- fix wstETH price fetch regression

* upgrade bridge clients

* explicitly upgrade ethers

* Jcf/element hotfixes (#1377)

- safe number to bigint conversion
- fix default element term apr when entering
- infer async from recipe, not interaction result

Co-authored-by: joss-aztec <[email protected]>
Co-authored-by: Lasse Herskind <[email protected]>
Co-authored-by: Jonathan Bursztyn <[email protected]>
Co-authored-by: Zachary James Williamson <[email protected]>
Co-authored-by: Josh <[email protected]>
Co-authored-by: iAmMichaelConnor <[email protected]>
Co-authored-by: Joe Andrews <[email protected]>
Co-authored-by: Leila Wang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants