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

Adding experimental online parser #2770

Merged
merged 5 commits into from
Sep 17, 2020

Conversation

thenamankumar
Copy link
Contributor

This PR includes implementation of an experimental online parser based on JSON rules set. This parser can be used with IDEs to implement syntax highlighting and auto suggestions. It is a state-full parser which parser the source string incrementally i.e. emits the next token each time.

@thenamankumar thenamankumar force-pushed the advanced-parser branch 2 times, most recently from a51166b to 608c3a7 Compare August 29, 2020 22:19
@IvanGoncharov IvanGoncharov added the PR: feature 🚀 requires increase of "minor" version number label Aug 29, 2020
@@ -0,0 +1,2 @@
export { Parser } from './parser';
export * from './types';
Copy link
Member

Choose a reason for hiding this comment

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

Can you please rename the folder to experimentalOnlineParser?

export interface LanguageType {
rules: Rules;
}
export interface Rules {
Copy link
Member

@IvanGoncharov IvanGoncharov Aug 30, 2020

Choose a reason for hiding this comment

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

Can you add GraphQLGrammar prefix to types for JSON ruleset?

export declare type Styles = {
[name: string]: string;
};
export declare type ParserConfig = {
Copy link
Member

Choose a reason for hiding this comment

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

For types related to parser can you please use OnlineParser prefix?

export declare type ParserConfig = {
tabSize: number;
};
export declare type ParserConfigOption = {
Copy link
Member

Choose a reason for hiding this comment

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

Can you please move Parser specific types into onlineParser.js?
and rename it to grammarTypes?

@@ -0,0 +1,2 @@
export { Parser } from './parser';
export * from './types';
Copy link
Member

@IvanGoncharov IvanGoncharov Aug 30, 2020

Choose a reason for hiding this comment

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

Please use explicit exports and expose only parser related types.

@@ -0,0 +1 @@
export { default } from './rules';
Copy link
Member

Choose a reason for hiding this comment

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

so just remove this file

export class Parser {
state: ParserState;
lexer: Lexer;
styles: Styles;
Copy link
Member

Choose a reason for hiding this comment

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

Can you please move it to GraphiQL code?

state: ParserState;
lexer: Lexer;
styles: Styles;
config: ParserConfig;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
config: ParserConfig;
_config: ParserConfig;


export declare class Parser {
state: ParserState;
lexer: Lexer;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
lexer: Lexer;
_lexer: Lexer;

} from './types';

export declare class Parser {
state: ParserState;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
state: ParserState;
_state: ParserState;

sol(): boolean;
parseToken(): Token;
indentation(): number;
private readonly parseTokenConstraint;
Copy link
Member

Choose a reason for hiding this comment

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

Can you please rename them and add just add underscore?

private readonly getNextRule;
private readonly popMatchedRule;
private readonly rollbackRule;
pushRule(
Copy link
Member

Choose a reason for hiding this comment

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

public methods should be grouped together.

state: ?ParserState,
styles: ?Styles,
config: ?ParserConfigOption,
source: string,
Copy link
Member

Choose a reason for hiding this comment

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

Nono-ptional parameters should go first.

styles,
config,
source,
}: {|
Copy link
Member

Choose a reason for hiding this comment

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

Can you please convert to positional arguments?
especially since we need to remove styles.

[name: string]: GraphQLGrammarRule;
}

export declare type GraphQLGrammarRule =
Copy link
Member

Choose a reason for hiding this comment

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

Please remove declare.

@@ -0,0 +1,76 @@
// @flow
Copy link
Member

Choose a reason for hiding this comment

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

Please remove these lines in all files

@@ -0,0 +1,938 @@
{
"rules": {
Copy link
Member

Choose a reason for hiding this comment

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

Please remove rules wrapper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might want to integrate this with ast later on or many include lex rules. So that is why I nested it.

Copy link
Member

Choose a reason for hiding this comment

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

@hereisnaman In this case rules and lexerRules would be confusing so we would need to rename it anyway.
It's a not a stable API so we can always introduce wrapper latter if needed.

@@ -0,0 +1,936 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

It looks like Deno doesn't support importing JSON:
denoland/deno#5633

Can you please convert it to rules.js and also merge it with grammarTypes.js?
On the plus side, we would be able to type-check rules content with TS.
Also please name the resulting file grammar.js.

Copy link
Member

Choose a reason for hiding this comment

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

If it triggers a coverage check please add necessary files to list of excluded files inside .nycrc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature 🚀 requires increase of "minor" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants