Team 2 - Converter

[mayank-ramnani]

Remade the PR because rebasing commits somehow introduced additional commits into the PR when changes were done that were suggested by the other team's review.
The review by the other team is in the old PR: #18

Directory structure of files modified in this PR:

- README.md # readme for the converter
- build.sh # builds the converter binary in debug-build/ folder with the name shadowdash
- src/ # contains the converter source code, previously contained ninja source code
- converter/
	- include/
		- manifest.h # contains definitions for shadowdash namespace that are required to compile the output of the converter into a library
	- testing/
		- example.build.ninja
		- example.zlib.ninja
	- run-tests.sh

For Reviewer:

• See README.md for how to build, run and test the converter.
• See the src/ directory for changes that were done to Ninja to make it behave as a converter.
• See the converter/ directory for test harness (run-tests.sh) and testcases.

[deflucaseng]

Commenting from Maksym Yemilianenko, Lucas Eng, and Daniel Torres

Strong points of your work!

The granularity of the commits remains high, with all of them being of similar sizes and serving an extremely clear purpose. The new series of commits has a much cleaner history. This group has done an excellent job with the granularity of adding isolated functionality to the converter with each of the commits.

The instructions for running the converter are clear and concise in the ReadMe

The code clearly deals with the edge cases it set out to fix related to the conversion of let statements in a much cleaner way now.

Continual Improvement

Per our last comment on the work, we believe it would be of benefit to add Unit Testing to the converter, as the only test currently is the ninja files
with ninja flags and usage, which is effective but could stand to be more comprehensive.

Conclusion

This group has done strong work improving certain edge cases and other related errors they faced previously, and will continue to stand to improve with more testing to their converter.

[kyotov]

Seems kind of scattered with too many changes?

Consider the points in https://docs.google.com/document/d/11e8zR3kcm7B-D6brAH_D4OgtYBEf94GZwQIXrnti5hM/edit?tab=t.0

Make appropriate changes to address above and let me know to look again.

[kyotov]

is this replacing the stock ninja readme!?

[mayank-ramnani]

Yes.

[kyotov]

why are you checking in an .so?

[mayank-ramnani]

Removed.

[1mpossible-code]

Team 4 Review

Change Granularity

the change granularity is good in our opinion, every change is structured and serves a specific purpose

Code Functionality

overall, the code meet the task requirements. it produces the build cc file and works pretty well. it also have a functional structure and seems not to include any useless parts in it.

Readability & Structure

everything is pretty much readable and understandable. sometimes the comments are missing here and there, but overall, the code is good and has a distinct purpose and you can see and follow the flow. the only thing is that the files can be definitely split up in much better way, addressing the purpose of filesystem. the util is very ambiguous and could benefit from distinction. other files have mixed responsibility so the structure can definitely be improved.
the readme file thing is the only concern that i am getting, because technically it is a subtask and should not recode the original README

Standards Compatibility

the style guides are followed and looks good overall.

Testing

the testing is something i want to address more. while some integrational (sort of) tests are present, the code is prone to have some issues because of lack of clear unit testing and that will lead to slowed down development and bigger bugs in the future. unit tests also stimulate a better structure and clearer organization of files which this project can benefit from.

[1mpossible-code]

if that is a part of the ninja like here, I believe rewriting the stock ninja readme is kinda wrong in this case

[1mpossible-code]

so this file is basically checking if the binary is compiling. and if it does compile it count the test as successful?
this is definitely not a unit test and it may be some sort of integration testing, but we will never know if some logic is forged or missing, because the test does not address the actual problems of the code. the code may not work, but still compile and that is my main concern and we will not figure this out unless the whole ShadowDash is working, but i would not put this code to production due to missing of some real test cases