Skip to content

Comments

LEST-71 - LuaLS annotations#46

Closed
yogwoggf wants to merge 73 commits intomasterfrom
LEST-71-LuaLS-annotations
Closed

LEST-71 - LuaLS annotations#46
yogwoggf wants to merge 73 commits intomasterfrom
LEST-71-LuaLS-annotations

Conversation

@yogwoggf
Copy link
Contributor

Summary:

  • Added a new package, luals-annotations, which is a TypeScript project.
  • Added Lest annotation generation using the docs from @lest/docs.
  • Added new top-level NPM commands to generate an annotation and save it to a file, and also to build the project.

@yogwoggf yogwoggf requested a review from Derpius June 28, 2023 19:25
@yogwoggf yogwoggf self-assigned this Jun 28, 2023
@yogwoggf yogwoggf force-pushed the LEST-71-LuaLS-annotations branch 3 times, most recently from ca7f183 to 608c664 Compare June 29, 2023 22:42
@yogwoggf yogwoggf marked this pull request as ready for review July 6, 2023 22:43
"./functions": "./dist/functions/index.js",
"./types": "./dist/types/index.js",
"./matchers": "./dist/matchers/index.js"
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The annotations requires this because it categorizes the docs by importing the individual folders instead of getting them through the main entrypoint. Also, it seems to require file extensions and only works with the NodeNext module resolution.

@Derpius
Copy link
Member

Derpius commented Jul 8, 2023

There's a couple things left that need resolving, after which I'll do a second pass:

There are a couple bits of logic I saw while resolving threads that are a little hard to parse, so you might want to do a quick self-review of your changes in case there's anything that you think could be improved still

@yogwoggf
Copy link
Contributor Author

yogwoggf commented Jul 9, 2023

FYI: In the second pass I assume you will look at the tests. Because the generated annotations aren't strict about whitespace/formatting, the tests look for "expected elements" and "unexpected elements." These effectively make sure that each annotation is a certain shape without considering whitespace or formatting.

@yogwoggf yogwoggf requested a review from Derpius July 9, 2023 20:15
@Derpius Derpius added the scope - docs Improvements or additions to documentation label Jul 16, 2023
Copy link
Member

@Derpius Derpius left a comment

Choose a reason for hiding this comment

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

Still haven't reviewed the tests yet, as there were still quite a few issues

Copy link
Member

Choose a reason for hiding this comment

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

Not a review comment, but you can actually make jest configs with typescript if you have ts-node as a dependency

testEnvironment: "node",
testMatch: ["**/*.test.ts"],
collectCoverage: true,
collectCoverageFrom: ["src/**/{!(index),}.ts"],
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we're excluding coverage for index files? Surely the tests should import from the indexes at least once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, another issue caused by a change when I had multiple index.ts files. index.ts can be considered a unit, but all it does is just combine all of the other tested units together to create the annotation file, it doesn't export any functions. It's pretty much the application.

"scripts": {
"start:ts-node": "ts-node src/index.ts",
"start": "node dist/src/index.js",
"dev": "npm run --debug start:ts-node && onchange \"src/**/*.ts\" -- npm run --debug start:ts-node",
Copy link
Member

Choose a reason for hiding this comment

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

nodemon works out the box with ts-node, but I've actually been using tsc-watch recently as ts-node has numerous compatibility issues (especially in a monorepo config)

Can leave this as is, but I recommend nodemon or tsc-watch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, ts-node is awful with monorepos, will look into a switch (nodemon/tsc-watch)

@yogwoggf
Copy link
Contributor Author

Still planning to fix these markups...

@Derpius
Copy link
Member

Derpius commented Oct 7, 2023

@yogwoggf when are you finishing this?

@yogwoggf
Copy link
Contributor Author

@yogwoggf when are you finishing this?

I'm not sure but it's 100% on the farthest backburner since Lest has had no activity since July 16th.

@Derpius Derpius closed this Dec 2, 2023
@yogwoggf yogwoggf deleted the LEST-71-LuaLS-annotations branch December 2, 2023 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope - docs Improvements or additions to documentation

Projects

Status: Closed

Development

Successfully merging this pull request may close these issues.

2 participants