Skip to content

team-4-new-language - #6

Open
waynewangyuxuan wants to merge 21 commits into
nyuoss:masterfrom
YuWei-CH:new-language
Open

team-4-new-language#6
waynewangyuxuan wants to merge 21 commits into
nyuoss:masterfrom
YuWei-CH:new-language

Conversation

@waynewangyuxuan

Copy link
Copy Markdown

No description provided.

@kyotov

kyotov commented Oct 9, 2024

Copy link
Copy Markdown

you also want to include an example that compiles against that manifest.h

@rzh4321

rzh4321 commented Nov 2, 2024

Copy link
Copy Markdown
  • You have clear variable names which makes it easy to follow. Your syntax is also intuitive and similar to ninja
  • You should add validation in your constructors. Your classes like rule and build should have logic in their constructors instead of leaving them empty
  • Your classes don't store their constructor parameters since they have no member variables. For example your var class doesn't have member variables "name" and "value" but your constructor requires those values to be passed
  • Your depth parameter in pool_ is never used. Your pool_ class also has inconsistent naming compared to your other classes without an underscore
  • You should add documentation or comments explaining each of your classes and what parameters they expect

@marwanabdelhameed

Copy link
Copy Markdown

Change Granularity:
Multiple commits per PR, which does not meet the requirements. Consider having it in one commit.
If not considering this, the commits are progressing in a logical sense with each commit serving a specific purpose.

Code Functionality:
Consider adding a builddir variable to control build output location.
The code does not handle Windows vs Unix command line differences
Other than that, the code matches what a build.ninja would look like.

Readability & Structure:
Some inconsistent naming conventions (pool_ and default_ vs other classes)
Well encapsulation of functions

Standards Compatibility:
Class fields are private, except for Token class
Consider adding error handling and exceptions.

Testing:
CircleCI is correctly set up and all the tests passes successfully

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.

8 participants