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

PR: code review #1

Open
wants to merge 122 commits into
base: before-pse
Choose a base branch
from
Open

PR: code review #1

wants to merge 122 commits into from

Conversation

jbessi
Copy link
Collaborator

@jbessi jbessi commented May 2, 2023

No description provided.

Copy link
Collaborator Author

@jbessi jbessi left a comment

Choose a reason for hiding this comment

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

DEPRECATED COMMENTS, IGNORE THEM

I didn't go deeply into code reviewing because of several reasons:

  • Please use the default directory structure we provided (f.e. move Monaco_editor and Parser to src directory).
  • Only use one Readme (the one provided in root folder). Add sections for Monaco_editor and Parser and be more detailed about how to set up the parser and editor.
  • Document the purpose of the files. It is hard to do a meaningful refactoring if it's unclear where to start. F.e. it's not clear how to run the test files or what the purpose of the file grammatik_idee is.
  • Also it's not clear to me, if there are deprecated files in this repository (like f.e. medcodelogic.pegjs and new_parser.pegjs).
  • It also looks like you didn't upload all of the files.

Please clean up the repo according to these comments and refactor into our provided folder structure.

Monaco_editor/ReadMe Outdated Show resolved Hide resolved
Monaco_editor/infoParser.js Outdated Show resolved Hide resolved
Monaco_editor/infoParserTest.html Outdated Show resolved Hide resolved
Parser/ReadMe Outdated Show resolved Hide resolved
Parser/grammatik_idee Outdated Show resolved Hide resolved
Parser/medcodelogic.pegjs Outdated Show resolved Hide resolved
Monaco_editor/main.html Outdated Show resolved Hide resolved
Parser/new_parser.pegjs Outdated Show resolved Hide resolved
Parser/variables.json 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.

6 participants