-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
feature: foundation for projects #223
Conversation
Codecov Report
@@ Coverage Diff @@
## master #223 +/- ##
==========================================
- Coverage 81.11% 81.05% -0.06%
==========================================
Files 182 189 +7
Lines 11996 12325 +329
==========================================
+ Hits 9730 9990 +260
- Misses 2266 2335 +69
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, well done. Most documentation is there, but there were a couple of functions that were a little unclear based on their signatures (asked for additional comments in those cases).
The one major issue I have is the lack of tests. The only thing that is tested w.r.t. projects is parsing of a correct manifest string. I know it's a bit tedious (and a lot of implementation still needs to be added for projects), but that doesn't mean that these foundations should not be tested.
Additionally, not writing tests moves the workload to the next person to touch this, which is not fair. It's our policy to ask contributors to add sufficient tests to their PRs, so we should do so ourselves too 🙂
Yes you are right. Do you suggest I add the test flow I described above? |
65528fe
to
5d04ec7
Compare
There are two approaches you can take. I think a lot of the functions you created actually lend themselves to unit tests. That would already bring a major improvement, I think. For the parts that can only be tested through integration tests, your proposed scheme looks good. The major functionalities for integration tests - which I can think of right now - are:
|
0b391ed
to
5b9103e
Compare
Ive added some more tests. The The crates |
5b9103e
to
31fcbb3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The additions of the tests make it look even more complete. Nice work! 👍
31fcbb3
to
f0b1a68
Compare
f0b1a68
to
38e497b
Compare
Relates to #203 |
This PR is an initial implementation of #203. It introduces the concept of a package as well as replacing the build step from the
mun
executable with a build step that expects amun.toml
to be present in the working directory (or one of its parents), similar to howcargo build
works. The goal of this PR is just to provide the foundation that we can build on in later PRs.Packages
The contents of the
mun.toml
should be similar to:The directory structure of a project should look like this
Multiple modules?!
This PR also adds multifile support, you can have multiple files in the source directory however there is no linking between them whatsoever, they are currently treated as separate libraries. (see below)
Whats missing?
There are a bunch of things missing from this PR that should be implemented at a later stage:
mun init
/mun new
command to initialize an empty package. (similar to cargo)mun build --watch
(or without--watch
)