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

Add initial implementation #1

Merged
merged 14 commits into from Aug 28, 2022
Merged

Add initial implementation #1

merged 14 commits into from Aug 28, 2022

Conversation

remcohaszing
Copy link
Member

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

This is the initial implementation of unist-lsp.

lib/index.js Show resolved Hide resolved
readme.md Show resolved Hide resolved
lib/index.js Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
.github/workflows/main.yml Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
test/index.js Outdated
} from 'unist-lsp'
import test from 'tape'

test('unistPointToLspPosition', async (t) => {
Copy link
Member

Choose a reason for hiding this comment

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

Probably good to tests missing props? null/undefined/invalid props?

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO it’s fine to rely on type information for this.

Copy link
Member

Choose a reason for hiding this comment

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

I won’t veto this but generally I believe that it is important to care for users who don’t use TypeScript.
I also believe that, for an open source library, it is important to write “defensively”. As in, assume people pass weird things in, and to an extent, handle that.
People do weird things in plugins. It’s good to not crash on weird things.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it depends a bit. I.e. when writing a remark plugin I tend to care about this more then when writing such low-level functions with a specific use cases like these.

tsconfig.json Show resolved Hide resolved
remcohaszing and others added 13 commits August 28, 2022 13:17
Co-authored-by: Titus <tituswormer@gmail.com>
Co-authored-by: Titus <tituswormer@gmail.com>
Co-authored-by: Titus <tituswormer@gmail.com>
Co-authored-by: Titus <tituswormer@gmail.com>
Co-authored-by: Titus <tituswormer@gmail.com>
Co-authored-by: Titus <tituswormer@gmail.com>
Co-authored-by: Titus <tituswormer@gmail.com>
@wooorm
Copy link
Member

wooorm commented Aug 28, 2022

@remcohaszing done?

@remcohaszing
Copy link
Member Author

Yep, IMO this is good to go. 👍

@wooorm
Copy link
Member

wooorm commented Aug 28, 2022

PS I do the following in settings on new repos:

  • turn wiki’s off
  • turn merge/rebase off
  • change squash to “default to pull request title” (I think this is new, will have to see if this works well)
  • turn on suggest updating branches, auto-merge, and auto-delete branches

@wooorm wooorm merged commit 2a72fed into main Aug 28, 2022
@wooorm wooorm deleted the initial-setup branch August 28, 2022 14:39
@github-actions
Copy link

Hi! This was closed. Team: If this was merged, please describe when this is likely to be released. Otherwise, please add one of the no/* labels.

@wooorm
Copy link
Member

wooorm commented Aug 28, 2022

Meh, github-tools/npm-tools pause after like 90 days or so. Reactivated them.

@wooorm
Copy link
Member

wooorm commented Aug 28, 2022

Ready for a release too I presume?

@remcohaszing
Copy link
Member Author

Meh, github-tools/npm-tools pause after like 90 days or so. Reactivated them.

Annoying!

Ready for a release too I presume?

Yep! Let’s make it version 1.0.0.

@wooorm
Copy link
Member

wooorm commented Aug 28, 2022

Are you sure it’s wise not to add a main to package.json. Despite what Node says in their docs, not everyone is using Node.
Not blocking to me, we can see if this works fine. But I’d err on the side of having it.

@wooorm
Copy link
Member

wooorm commented Aug 28, 2022

Also, is rimraf no longer needed for tsc?

@wooorm
Copy link
Member

wooorm commented Aug 28, 2022

Bundlephobia is reporting a dep is missing: https://bundlephobia.com/package/unist-lsp@1.0.0.
Not sure which one, and why?

@remcohaszing
Copy link
Member Author

Also, is rimraf no longer needed for tsc?

I’m not running into any issues anyway.

Bundlephobia is reporting a dep is missing: https://bundlephobia.com/package/unist-lsp@1.0.0. Not sure which one, and why?

Oops! #2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants