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

Team4-Converter #25

Open
wants to merge 11 commits into
base: team4-converter
Choose a base branch
from

Conversation

waynewangyuxuan
Copy link

No description provided.

@1mpossible-code 1mpossible-code mentioned this pull request Dec 7, 2024
@1mpossible-code
Copy link

1mpossible-code commented Dec 7, 2024

README.md contains all the information regarding the implementation, but we will include the help message below:

$ python3 converter.py --help
usage: converter.py [-h] input_file output_file

Converts ninja files to ShadowDash manifest files.

positional arguments:
  input_file   Path to the input file to process
  output_file  Path to the output file

options:
  -h, --help   show this help message and exit

We addressed the feedback:

  • each file has a purpose and there are no unnecessary files in the PR
  • the original gitignore of ninja was enough not to include any new files to be ignored
  • there are no secrets committed
  • the API description is defined in in README.md and as a --help argument parameter of the main (converter.py) file
    • API was designed to be as simple and as clear as possible

Tests

Unit tests are presented in the ./converter/tests/ directory. To run tests execute pytest command from the ./converter directory.

@1mpossible-code
Copy link

@kyotov added tests.

Choose a reason for hiding this comment

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

The files & coding generally have appropriate granularity, though some functions could be further decomposed. For example, the main function can be updated to decouple the file handling from token parsing and file writing, it would allow main() to act as a lightweight wrapper and keeps processing logic reusable.

Copy link

@grace-chen16 grace-chen16 Dec 16, 2024

Choose a reason for hiding this comment

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

All the test files were put into a single folder, enabling Pytest to efficiently run tests on them, showcasing a testing friendly design.

@grace-chen16
Copy link

Granularity:
The files generally have appropriate granularity, though some functions could be further decomposed.
• For example, the main function tightly couples file IO with parsing and building. This makes it difficult to test the core logic without filesystem dependencies. If the codes can decouple the file handling from token parsing and file writing, it would allow main() to act as a lightweight wrapper and keeps processing logic reusable.

Functionality:
The code appears to function as intended; however scalability can be improved:
converter.py:
The primary purpose of converter.py is to read a Ninja build file and refactor its content into C++ code in a format that can be compiled (e.g., ninja.build.cc).
This involves parsing command-line arguments to determine the paths of the input and output files. Uses the Parser.tokenize method to convert the Ninja build file into a list of tokens. And then passes the tokenized data to Builder.get_file_content, which creates the final content of the ShadowDash manifest. And finally writes the generated content to the specified output file.
Supporting Components:
parser.py: Parses the Ninja build file and converts it into a list of tokens (Token objects) based on its content and structure.
builder.py: Generates the ShadowDash manifest content based on the list of tokens provided by parser.py.
token.py: Defines the Token class and its subclasses to represent different constructs in the Ninja build file, with each subclass is tailored to handle specific commands like rule, build, bind, and pool.

Scalability:
The code can generate build.ninja.cc file on build.ninja example for hello.cc. However seems still needs more work to be able to convert more complex build.ninja files such as the ones generated by CMake for larger build projects like Zlib for LLVM.

Readability:
Overall, readability is good, supporting functions are well organized.
Standards Compliance:
Good in general.
Testing:
Team 4 organized all the test files into a single folder, enabling Pytest to efficiently run tests on them, showcasing a testing friendly design.
Overall test was also performed and build.ninja.cc can be successfully generated for simple build.ninja file (example build file provided for hello.cc). However it seems needs more work to be able to convert from CMake generated build.ninja file for larger build project like (Zlib or LLVM).

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.

5 participants