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

Grammar helper #93

Merged
merged 20 commits into from
Aug 22, 2023
Merged

Grammar helper #93

merged 20 commits into from
Aug 22, 2023

Conversation

mindplay-dk
Copy link
Contributor

Per #85 this is ready for review. 🙂

@vercel
Copy link

vercel bot commented Aug 16, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sigma ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 22, 2023 8:18pm

@mindplay-dk
Copy link
Contributor Author

The commit-log is a bit of a mess - feel free to squash the whole thing if that makes you happy. 😁

@coveralls
Copy link

Pull Request Test Coverage Report for Build 5879923495

  • 6 of 7 (85.71%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.3%) to 99.721%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/core/grammar.ts 6 7 85.71%
Totals Coverage Status
Change from base Build 5869337619: -0.3%
Covered Lines: 271
Relevant Lines: 272

💛 - Coveralls

@mindplay-dk
Copy link
Contributor Author

That one uncovered line is the throw new Error statement, which is not supposed to execute.

I don't know coveralls, and they don't seem to have any real docs - do you know how to mark the line for exclusion?

I guess, ideally, the line shouldn't be there - I just feel safer having something that blows up, if someone breaks the code, rather than a silent failure... 🤔

@norskeld
Copy link
Owner

Thanks, I'll get to this PR on weekends.

@norskeld norskeld linked an issue Aug 19, 2023 that may be closed by this pull request
Copy link
Owner

@norskeld norskeld left a comment

Choose a reason for hiding this comment

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

Overall looks good, but some minor changes wouldn't hurt.

docs/docs/content/core/grammar.md Outdated Show resolved Hide resolved
docs/docs/content/core/grammar.md Outdated Show resolved Hide resolved
src/core/grammar.ts Outdated Show resolved Hide resolved
src/core/grammar.ts Outdated Show resolved Hide resolved
src/core/grammar.ts Outdated Show resolved Hide resolved
src/core/grammar.ts Show resolved Hide resolved
src/__tests__/core/grammar.spec.ts Outdated Show resolved Hide resolved
src/core/grammar.ts Show resolved Hide resolved
mindplay-dk and others added 2 commits August 20, 2023 10:19
Co-authored-by: Vladislav Mamon <[email protected]>
Co-authored-by: Vladislav Mamon <[email protected]>
@mindplay-dk
Copy link
Contributor Author

By the way, quick question, do you have a reason for having both the hand-written markdown, as well as the very similar inline documentation in the classes?

I'm a bit concerned about the amount of duplication here, the risk of divergence, and so on. I just noticed my inline and markdown docs+example are already quite different.

I wonder if it would make more sense to extract the markdown from the inline doc-blocks?

@norskeld
Copy link
Owner

By the way, quick question, do you have a reason for having both the hand-written markdown, as well as the very similar inline documentation in the classes?

I'm a bit concerned about the amount of duplication here, the risk of divergence, and so on. I just noticed my inline and markdown docs+example are already quite different.

I wonder if it would make more sense to extract the markdown from the inline doc-blocks?

We looked into this earlier with @TheFedaikin, but decided to leave it as-is, because:

- I'm not a fan of TypeDoc and the like.

- I like VitePress and we couldn't find a nice way to wire docs to the code. VitePress allows to import code snippets from files, but it doesn't let you import comments.

The right way would be to write a custom pre-build step for docs where we extract TSDocs and generate Markdown files, but this has its own pros/cons/tradeoffs. For instance, we would have to drop all bells and whistles VitePress gives us: badges, nice codeblocks, custom success/failure blocks and so on, because all of that wouldn't render correctly in inline docs.

- We would need to author documentation in the comments, which kinda sucks compared to good old Markdown that can be previewed in real-time.

- I wanted inline docs to only have the basic info and standalone docs to provide detailed information. Having identical inline and standalone docs kinda defeats the purpose of having standalone docs in the first place.

@mindplay-dk
Copy link
Contributor Author

FYI, there is a markdown plugin for typedoc with 250K weekly downloads. (I've looked at every other documentation generator I could find, plus at least 5 different generators designed specifically for Markdown, but I didn't find anything else that looks very dependable, maintained, complete, popular, etc.)

I wanted inline docs to only have the basic info and standalone docs to provide detailed information. Having identical inline and standalone docs kinda defeats the purpose of having standalone docs in the first place.

To my mind, docs are docs. I don't see any benefits to having so-so inline documentation, and then having to visit a website to get the whole story. I don't see any practical benefits to having two versions of documentation for the same things.

Personally, I would try to solve those issues with TSDoc instead - add plugins, customize the theme, use inline HTML if necessary... to each his own though. Migrating it at this point would be a project in itself, I suppose. 😅

@norskeld
Copy link
Owner

FYI, there is a markdown plugin for typedoc with 250K weekly downloads. (I've looked at every other documentation generator I could find, plus at least 5 different generators designed specifically for Markdown, but I didn't find anything else that looks very dependable, maintained, complete, popular, etc.)

I wanted inline docs to only have the basic info and standalone docs to provide detailed information. Having identical inline and standalone docs kinda defeats the purpose of having standalone docs in the first place.

To my mind, docs are docs. I don't see any benefits to having so-so inline documentation, and then having to visit a website to get the whole story. I don't see any practical benefits to having two versions of documentation for the same things.

Personally, I would try to solve those issues with TSDoc instead - add plugins, customize the theme, use inline HTML if necessary... to each his own though. Migrating it at this point would be a project in itself, I suppose. 😅

Thanks for the insight, maybe I'll reconsider.

@norskeld norskeld merged commit e8074be into norskeld:master Aug 22, 2023
2 checks passed
norskeld pushed a commit that referenced this pull request Aug 22, 2023
# [3.7.0](v3.6.5...v3.7.0) (2023-08-22)

### Features

* add grammar helper ([#93](#93)) ([e8074be](e8074be))
@norskeld
Copy link
Owner

This PR is included in version 3.7.0.

@norskeld norskeld added the R-released Semantic Release: Success label Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R-released Semantic Release: Success
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: grammar helper
3 participants