Skip to content

Add CSP and other files to explorer #168

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

Merged
merged 10 commits into from
Jul 1, 2020

Conversation

ty-d
Copy link
Contributor

@ty-d ty-d commented Jun 26, 2020

Add "CSP Files" and "Other" section to the ObjectScript explorer, and support compilation of those files. The studio actions editor context menu was modified to appear for CSP and other files.

@ty-d ty-d requested a review from daimor as a code owner June 26, 2020 18:41
@ty-d ty-d force-pushed the CSPandOtherFiles branch from d773f3b to e2fcbaf Compare June 26, 2020 19:00
@ty-d ty-d requested review from gjsjohnmurray and isc-rsingh June 29, 2020 13:03
Copy link
Contributor

@gjsjohnmurray gjsjohnmurray left a comment

Choose a reason for hiding this comment

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

@ty-d how about fixing the linting warnings about missing return types in index.ts while you are touching it?

@gjsjohnmurray
Copy link
Contributor

@ty-d how about fixing the linting warnings about missing return types in index.ts while you are touching it?

Looks like @daimor is already dealing with these in #172

@ty-d ty-d requested a review from gjsjohnmurray June 29, 2020 17:55
Copy link
Contributor

@gjsjohnmurray gjsjohnmurray left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -42,14 +51,20 @@ export class RootNode extends NodeBase {
spec = "*.cls";
break;
case "RTN":
spec = "*.mac,*.int";
spec = "*.mac,*.int,*.bas";
Copy link
Member

Choose a reason for hiding this comment

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

Is this really necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not?

@@ -61,10 +61,17 @@ export class DocumentContentProvider implements vscode.TextDocumentContentProvid
});
}
}
const isCsp = name.split("/")[0] === "csp";
Copy link
Member

Choose a reason for hiding this comment

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

No, that's not a rule. CSP application may have any name. You need a better way to check if it's a CSP.

case "CSP":
spec = "*";
break;
case "OTHER":
Copy link
Member

Choose a reason for hiding this comment

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

OTHER replace everywhere with OTH, to be consistent with Atelier API

@isc-rsingh
Copy link
Member

@daimor GitHub is saying you have requested changes, but I think your request has been addressed. Are we ready to merge this PR?

@isc-rsingh isc-rsingh merged commit 74cfce9 into intersystems-community:master Jul 1, 2020
@ty-d ty-d deleted the CSPandOtherFiles branch July 2, 2020 13:22
@ty-d ty-d restored the CSPandOtherFiles branch July 2, 2020 13:22
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.

6 participants