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

Import enhancement #172

Merged
merged 18 commits into from
Sep 4, 2020
Merged

Import enhancement #172

merged 18 commits into from
Sep 4, 2020

Conversation

CGQAQ
Copy link
Contributor

@CGQAQ CGQAQ commented Sep 1, 2020

REWRITE OF #115 DUE TO REPO BASE REPLACEMENT

  • import snippets (impstd, impx)
  • import std modules version completion
  • import x modules version completion
  • import std modules file/path completion
  • import x modules file/path completion
  • cache the completion result
  • x module name completion
  • update cache by tracking the uploaded_at from this API
  • clear cache function in ctrl + shift + p menu

Showcase:

Completely redesign completion

Before

d

After

h

closes #114

@jsejcksn

This comment has been minimized.

@CGQAQ CGQAQ force-pushed the import_enhancement branch 3 times, most recently from 6614e1c to 9e52604 Compare September 1, 2020 10:07
@CGQAQ

This comment has been minimized.

@CGQAQ

This comment has been minimized.

@lucacasonato
Copy link
Member

@lucacasonato

Sorry for bothering you,

https://api.deno.land/modules?limit=10&query=$QUERY this API how to let result sort by alphabetical order

image

default is sort by search_score, I cant filter completions by name

image

And also where is the docs about these API, I can't find it

There are no docs. Also there is no option for alphabetical sort.

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

I tried this out locally and it is working great so far. Very awesome feature @CGQAQ!

client/src/import_utils.ts Outdated Show resolved Hide resolved
snippets/import_enhancement.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
client/src/import_utils.ts Outdated Show resolved Hide resolved
client/src/import_enhancement_provider.ts Outdated Show resolved Hide resolved
@CGQAQ
Copy link
Contributor Author

CGQAQ commented Sep 3, 2020

Don't have internet right now, the typhoon is coming. I'll change it as soon as the internet back

@CGQAQ CGQAQ marked this pull request as ready for review September 4, 2020 09:31
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

Some other minor comments:

  • Autocomplete only works correctly if @ and / for path are present. Example:
import { } from "https://deno.land/x/sq" // intellisense does not complete this to sqs
import { } from "https://deno.land/x/" // intellisense does work for one round now
import { } from "https://deno.land/x/sqs@" // intellisense does not suggest versions now
import { } from "https://deno.land/x/sqs@/" // intellisense does suggest versions now between @ and /
  • Not specifying a version means path completion doesn't work (I am actually ok with this, maybe people will stop using it then 😄)

  • Typing some characters of the module name does not retrigger search. You have to manually retrigger with Esc then Ctrl + Space - it would be great if typing a char would trigger the search at the server to happen again. (This is fine for the first pass, but if you know how to solve it that would be awesome).

client/src/import_enhancement_provider.ts Show resolved Hide resolved
client/src/import_enhancement_provider.ts Show resolved Hide resolved
client/src/import_utils.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@CGQAQ
Copy link
Contributor Author

CGQAQ commented Sep 4, 2020

Some other minor comments:

  • Autocomplete only works correctly if @ and / for path are present. Example:
import { } from "https://deno.land/x/sq" // intellisense does not complete this to sqs
import { } from "https://deno.land/x/" // intellisense does work for one round now
import { } from "https://deno.land/x/sqs@" // intellisense does not suggest versions now
import { } from "https://deno.land/x/sqs@/" // intellisense does suggest versions now between @ and /
  • Not specifying a version means path completion doesn't work (I am actually ok with this, maybe people will stop using it then 😄)
  • Typing some characters of the module name does not retrigger search. You have to manually retrigger with Esc then Ctrl + Space - it would be great if typing a char would trigger the search at the server to happen again. (This is fine for the first pass, but if you know how to solve it that would be awesome).

@lucacasonato I have completely rewrite the completion logic
h

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

Thanks a lot! This is a really awesome feature to have :-)

@CGQAQ
Copy link
Contributor Author

CGQAQ commented Sep 4, 2020

Thanks a lot! This is a really awesome feature to have :-)

Thank you for helping me to have this done, also great thanks to @jrieken from Microsoft for helping me out

@lucacasonato lucacasonato merged commit dd25cff into denoland:master Sep 4, 2020
@axetroy
Copy link
Contributor

axetroy commented Sep 5, 2020

@CGQAQ You did a great job 👍

This function should be moved to the server, not the client for better performance.

@CGQAQ If you are still interested, you can try to move it to core and write test cases.

Then implement in server

@CGQAQ
Copy link
Contributor Author

CGQAQ commented Sep 5, 2020

@CGQAQ You did a great job

This function should be moved to the server, not the client for better performance.

@CGQAQ If you are still interested, you can try to move it to core and write test cases.

Then implement in server

I'll read the server code later and see what I can do. (话说你有qq开发群吗?这里讨论不是很方便)

@axetroy
Copy link
Contributor

axetroy commented Sep 5, 2020

I'll read the server code later and see what I can do. (话说你有qq开发群吗?这里讨论不是很方便)

以前有,现在没有精力去搞这种了,Github 就是很好的交流社区

@CGQAQ
Copy link
Contributor Author

CGQAQ commented Sep 5, 2020

I'll read the server code later and see what I can do. (话说你有qq开发群吗?这里讨论不是很方便)

以前有,现在没有精力去搞这种了,Github 就是很好的交流社区

可惜没有私聊,评论太多不好,感觉性能提升不了多少,毕竟有http请求,不过体验应该会更好,而cache过的在client端的实现也看不出延迟

@CGQAQ
Copy link
Contributor Author

CGQAQ commented Sep 5, 2020

@axetroy what is this for?

axetroy pushed a commit to axetroy/vscode_deno that referenced this pull request Sep 5, 2020
@axetroy
Copy link
Contributor

axetroy commented Sep 5, 2020

@axetroy what is this for?

Redundant code, just remove it.

@axetroy
Copy link
Contributor

axetroy commented Sep 5, 2020

and BTW.

There is already a cache module and it has been well tested.

See

/**
* Cache module, cache data
*/
export class Cache<T> {
/**
* create a cache module
* @param timeout How long will this data expire. If expired, return undefined. eg 1000ms
* @param allowReferenceTimes How many times the data can be referenced.If 0, no restrictions
*/
static create<T>(timeout: number, allowReferenceTimes?: number): Cache<T> {
return new Cache<T>(timeout, allowReferenceTimes);
}

@CGQAQ
Copy link
Contributor Author

CGQAQ commented Sep 5, 2020

and BTW.

There is already a cache module and it has been well tested.

See

/**
* Cache module, cache data
*/
export class Cache<T> {
/**
* create a cache module
* @param timeout How long will this data expire. If expired, return undefined. eg 1000ms
* @param allowReferenceTimes How many times the data can be referenced.If 0, no restrictions
*/
static create<T>(timeout: number, allowReferenceTimes?: number): Cache<T> {
return new Cache<T>(timeout, allowReferenceTimes);
}

Ok, I'll remove vscode-cache dep and use it, thank you

CGQAQ added a commit to CGQAQ/vscode_deno that referenced this pull request Sep 5, 2020
)"

This reverts commit dd25cff.
Don't need these now, will move to server
@CGQAQ
Copy link
Contributor Author

CGQAQ commented Sep 5, 2020

and BTW.

There is already a cache module and it has been well tested.

See

/**
* Cache module, cache data
*/
export class Cache<T> {
/**
* create a cache module
* @param timeout How long will this data expire. If expired, return undefined. eg 1000ms
* @param allowReferenceTimes How many times the data can be referenced.If 0, no restrictions
*/
static create<T>(timeout: number, allowReferenceTimes?: number): Cache<T> {
return new Cache<T>(timeout, allowReferenceTimes);
}

@axetroy but how to permanent the cache to disk

@axetroy
Copy link
Contributor

axetroy commented Sep 5, 2020

@CGQAQ This is only a memory cache. If you need to cache to the disk, you can implement it yourself or use a third-party library

@CGQAQ
Copy link
Contributor Author

CGQAQ commented Sep 5, 2020

@CGQAQ This is only a memory cache. If you need to cache to the disk, you can implement it yourself or use a third-party library

which folder should I put? do you have any suggestions? I will put it in vscode_deno/cache at the same location of deno folder

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.

Add some way to quickly import std modules
4 participants