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 a service to generate bnf/diagrams #7

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

Conversation

premkumr
Copy link

@premkumr premkumr commented Aug 4, 2023

Server

  • Starts a service with --server
  • make builds the code
  • make server starts the server in debug mode with sample ebnf file
  • eg: curl "localhost:1314/bnf?mode=diagram&rules=select"

Request params

  • api : ysql/ycql (Default: ysql)
  • version: preview/stable/v2.12 ... (Default: preview)
  • mode : reference/grammar/diagram
  • depth : number (optional param)
  • rules : comma separated rule names (eg rules=select,select_start)
  • local: comma separated rule names which will have local references (eg rules=select,select_start)

Tech spec

Copy link

@jasonyb jasonyb left a comment

Choose a reason for hiding this comment

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

Before further review, could the changes be better documented in the PR (such as high-level approach), and could the README be updated with the new interface?

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/main/java/net/nextencia/rrdiagram/Main.java Outdated Show resolved Hide resolved
src/main/java/net/nextencia/rrdiagram/Server.java Outdated Show resolved Hide resolved
src/main/java/net/nextencia/rrdiagram/BNFProcessor.java Outdated Show resolved Hide resolved
src/main/java/net/nextencia/rrdiagram/BNFProcessor.java Outdated Show resolved Hide resolved
src/main/java/net/nextencia/rrdiagram/BNFProcessor.java Outdated Show resolved Hide resolved
src/main/java/net/nextencia/rrdiagram/BNFProcessor.java Outdated Show resolved Hide resolved
ysql_grammar.ebnf Show resolved Hide resolved
Copy link

@jasonyb jasonyb left a comment

Choose a reason for hiding this comment

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

partial review

.gitignore Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/main/java/net/nextencia/rrdiagram/BNFProcessor.java Outdated Show resolved Hide resolved
ysql_grammar.ebnf Show resolved Hide resolved
@premkumr
Copy link
Author

Before further review, could the changes be better documented in the PR (such as high-level approach), and could the README be updated with the new interface?

Added detailed Tech Spec

@premkumr
Copy link
Author

@jasonyb - Please let me know if you need ore changes !

Copy link

@jasonyb jasonyb left a comment

Choose a reason for hiding this comment

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

why is ysql_grammar.ebnf added?

Makefile Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
src/main/java/net/nextencia/rrdiagram/BNFProcessor.java Outdated Show resolved Hide resolved
src/main/java/net/nextencia/rrdiagram/BNFProcessor.java Outdated Show resolved Hide resolved
src/main/java/net/nextencia/rrdiagram/BNFProcessor.java Outdated Show resolved Hide resolved
src/main/java/net/nextencia/rrdiagram/BNFProcessor.java Outdated Show resolved Hide resolved
src/main/java/net/nextencia/rrdiagram/BNFProcessor.java Outdated Show resolved Hide resolved
src/main/java/net/nextencia/rrdiagram/BNFProcessor.java Outdated Show resolved Hide resolved
@premkumr
Copy link
Author

premkumr commented Oct 2, 2023

why is ysql_grammar.ebnf added?

For testing. make server uses this file

@premkumr premkumr requested a review from jasonyb October 2, 2023 15:42
Copy link

@jasonyb jasonyb left a comment

Choose a reason for hiding this comment

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

Over half the comments are not addressed.

why is ysql_grammar.ebnf added?

For testing. make server uses this file

When you say testing, do you mean manual testing? What does make test do? If it actually runs tests, isn't it not server tests? Then you are still missing tests for the server. Not that you necessarily need it in the early stage.

Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
src/main/java/net/nextencia/rrdiagram/BNFProcessor.java Outdated Show resolved Hide resolved
src/main/java/net/nextencia/rrdiagram/BNFProcessor.java Outdated Show resolved Hide resolved
@premkumr
Copy link
Author

premkumr commented Oct 3, 2023

Over half the comments are not addressed.

@jasonyb - Not sure why this happened. They were all "hidden", I had to click to expand them. Only seeing them now. Will address them and then ping you.

Copy link
Author

@premkumr premkumr left a comment

Choose a reason for hiding this comment

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

@jasonyb - let me know how this looks.

src/main/java/net/nextencia/rrdiagram/Server.java Outdated Show resolved Hide resolved
src/main/java/net/nextencia/rrdiagram/Server.java Outdated Show resolved Hide resolved
src/main/java/net/nextencia/rrdiagram/Server.java Outdated Show resolved Hide resolved
src/main/java/net/nextencia/rrdiagram/Server.java Outdated Show resolved Hide resolved
src/main/java/net/nextencia/rrdiagram/Server.java Outdated Show resolved Hide resolved
src/main/java/net/nextencia/rrdiagram/Server.java Outdated Show resolved Hide resolved
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