Skip to content
This repository has been archived by the owner on Nov 16, 2021. It is now read-only.

RIP 0.1 Reach TypeScript Design Style Request #39

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

KBryan
Copy link
Contributor

@KBryan KBryan commented Jun 26, 2021

There are areas in the code where the same string is used multiple times. I think it would be a good idea to start making these const wherever possible.

@KBryan
Copy link
Contributor Author

KBryan commented Jun 30, 2021

Needs UnitTests for handling the string interpolation in VSCode notice some funny behaviour and documentation for creating additional plug-ins.

@@ -0,0 +1,29 @@
export enum CMD_HELPER {
COMPILE = <any>'compile',
Copy link
Contributor

Choose a reason for hiding this comment

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

why <any>?

@@ -1,6 +1,15 @@
import { ExtensionContext } from 'vscode';
import * as vscode from 'vscode';

const REACH_COMPILE_S:string = 'Reach Compile';
Copy link
Contributor

Choose a reason for hiding this comment

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

same comments as previous PR

@@ -0,0 +1,32 @@
export class CONSTANTS {
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer only things that are used more than once are abstracted to constants. Otherwise, I find it easier to have the values inlined

public static CANNOT_CREATE_SETTINGS:string = `Could not create .vscode/settings.json:`;
public static REACH_PATH:string = './reach';
public static REACH_CLIENT_ID:string = 'reachide';
public static CREATE_FILESYSTEM_WATCHER_PARAM:string = '**/*.rsh';
Copy link
Contributor

Choose a reason for hiding this comment

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

public static RECURSIVE_REACH_FILE_SELECTOR : string = '**/*.rsh'

public static fs = require('fs');
public static url = require('url');

public static REACH_IDE_S:string = 'Reach IDE';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: drop the _S

cmdHelper(value);
}

urlHelper(context, URL_HELPER.DOCS, URL_HELPER.DOCS_URL);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just make this:

const helperUrls = {
  'docs': 'https://docs.reach.sh/doc-index.html',
  // ...
}

for (const [key, value] of Object.entries(helperUrls)) {
  urlHelper(context, key, value);
}

@KBryan
Copy link
Contributor Author

KBryan commented Jun 30, 2021

Thanks for the input valid points. I will add these style changes and resubmit

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

Successfully merging this pull request may close these issues.

2 participants