-
Notifications
You must be signed in to change notification settings - Fork 1
Description
Disclaimer
All of what is said in this issue is a recommendation only. Please don't take it personally as berating or degrading the repository. If you disagree with anything that is said here, contact me through this issue. I am not a rust expert. I will like to be corrected in case I was wrong.
Sometime I will provide links for stuff I want you to look at and sometimes I won't. Take your time and look up what you don't understand.
Overview
In this "CR" I will not discuss any implementation detail (i.e. I don't care about how you implement the). Nor do I criticize how you write your code. I'll leave that for you to learn alone. Overall you did a great job, I like what I see, but it can be improved. Hope you'll have fun reading this. Good luck :)
The Rust Ecosystem
Tests
The Rust ecosystem provides a great way to test your code. There is no reason to have a tests folder. Every workspace (e.g. xta-analyzer) can have a tests modules in it's lib.rs file. You can then use cargo to run them. This will also keep you from putting garbage in your main.rs file. Look it up.
Modules
Rust provides a great way to separate logical components (e.g. structs) into multiple physical resources (e.g. files) without changing the way you use them. Please read this part of the rust book.
Builtins
-
Rust have an
Iteratorpattern built in
Your Scanner can be treated as an iterator. It will unlock some cool features you don't have because you don't implement a simple trait. Look it up. -
Macros Macros Macros
In Some places you use builtin macros, in others you don't. You use macros likewrite!()andmatches!(). It shows that you done some research. But your code would benefit from others. Inxta-analyzer/src/analyzer.rsyou have an unimplemented method namesnew. Instead of leaving a todo comment you could've used thetodo!()macro (or theunimplemented!()macro). It states that the function isn't implemented and reduces the number of errors in your editor.
Git Branches
Branches are a must
While you do use git branches. You aren't really consistent with how you name them. Choose a convention and stick with it religiously. Do not commit straight to the main branch. First develop on the feature's branch then let it enter dev.
Branches Have a Semantic meaning
Regardless of the naming convention, the branch name should signal it's purpose. When I see a main branch, I expect it to contain a runnable "production ready" application (In quotes because I don't expect this to be a gcc like product, but at least something I can run and muddle with). The program in your main branch doesn't even compile. In contrast the dev branch should contain every stable version in your application. It should (almost) always be ahead on the main branch.
"Clean Code"
Although clean code rules are not set in stone and you should decide some for yourself.
Comments
Personally, do not like comments. Your code should speak for itself
Naming
The naming of struct, enums, enum values, functions (everything) should be self-explaining. The extra letters don't cost you money. Take for example the Loc struct. What does it mean? Lock? Local? Lab On Chip? Library of Congress? No, it means Location. If I hadn't checked I would've never known.
Values in the TokenKind enum are named inconsistently. When naming the Identifier you took the liberty and wrote the full name. However, when naming Min (which I assumed means Minus) you didn't. Same with Equals and LowerOrEqu. Choose one and go with it. I suggest naming fully.
Managing Components
I recommend having each component (e.g. struct, enum) have it's own file. The Scanner struct is places solely in the scanner.rs file, good. The token.rs though, contains the Loc struct, it's Impl; The Token struct and it's Impl; The TokenKind enum which is huge by itself and the lookup_keyword function. This repeats in parser.rs, ast.rs, analyzer.rs and scope.rs. You can use rust modules to make it better. Choose something that you'd like (I guess something like each struct or enum and it's impl in a file is fine) and stick with it.
Use a Linting Tool
It's overall fine but I do see some inconsistencies with this. Use a linter that will format your files to follow the same conventions.