Skip to content

Refactor extension.ts#525

Merged
Ikuyadeu merged 2 commits intoREditorSupport:masterfrom
ManuelHentschel:cleanup
Jan 17, 2021
Merged

Refactor extension.ts#525
Ikuyadeu merged 2 commits intoREditorSupport:masterfrom
ManuelHentschel:cleanup

Conversation

@ManuelHentschel
Copy link
Copy Markdown
Member

What problem did you solve?
Implements the refactoring suggested in #479.

How can I check this pull request?
This PR is not intended to change the behavior of the extension.

I also changed the imports at the top of extension.ts to namespace imports:

  • Improves the readibility of the code
  • Makes it easier to keep track of where each function comes from
  • Makes life easier for developers. E.g.
    • you can use intellisense to access imported functions (e.g. vscode. + ctrl+space to view all exported variables from vscode)
    • it's easier to move code around between files (you only need to add/remove the namespace imports sometimes, previously you had to add/remove each function individually)
    • Fewer (redundant) changes in Git diffs

If you disagree, this can of course be reverted, but I'd strongly suggest using namespace imports here.

@Ikuyadeu
Copy link
Copy Markdown
Member

Improves the readibility of the code
Makes it easier to keep track of where each function comes from

@ManuelHentschel Great works! Our source code looks more cleaned.

you can use intellisense to access imported functions (e.g. vscode. + ctrl+space to view all exported variables from vscode)
it's easier to move code around between files (you only need to add/remove the namespace imports sometimes, previously you had to add/remove each function individually)

Good, I denied it before, but it may be better to change the way of thinking.

Fewer (redundant) changes in Git diffs

In other words, we can see the files that got the impact of the main changes.
How about using an asterisk for external packages such as vscode and individual imports for vscode -r packages such as session and util?

Comment thread src/rTerminal.ts
@ManuelHentschel
Copy link
Copy Markdown
Member Author

How about using an asterisk for external packages such as vscode and individual imports for vscode -r packages such as session and util?

That sounds like a good rule of thumb, although I would still prefer to use namespace imports for vscode-r files when the number of imported functions is rather large (as e.g. in extension.ts).

@Ikuyadeu
Copy link
Copy Markdown
Member

@ManuelHentschel I agree with you.
And I'll update flexible eslint rules after.
Link: https://eslint.org/docs/rules/no-restricted-imports

Anyway, I'll merge the PR that is enough good.

@Ikuyadeu Ikuyadeu merged commit 16737cb into REditorSupport:master Jan 17, 2021
@ManuelHentschel ManuelHentschel deleted the cleanup branch January 17, 2021 15:11
@Ikuyadeu Ikuyadeu mentioned this pull request Jun 30, 2021
4 tasks
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.

2 participants