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

feature: foundation for projects #223

Merged
merged 2 commits into from
Jun 24, 2020

Conversation

baszalmstra
Copy link
Collaborator

@baszalmstra baszalmstra commented Jun 12, 2020

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 a mun.toml to be present in the working directory (or one of its parents), similar to how cargo 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:

[package]
name="foobar"
author=["John Doe"]
version="0.1.0"

The directory structure of a project should look like this

package_dir
|- src
|   \- main.mun
\- mun.toml

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:

  • Update the docs (because a project is now required)
  • We should add a mun init/mun new command to initialize an empty package. (similar to cargo)
  • Modules should be able to interact with each other and be compiled to a single package (depending on hotreload settings).
  • Add proper integration tests where we actually test:
    1. create a project
    2. run mun build --watch (or without --watch)
    3. load the binary and invoke it (testing its functionality)
    4. modify the source
    5. test the functionality again

@baszalmstra baszalmstra changed the title feature: compiling of projects instead of files feature: foundation for projects Jun 12, 2020
@baszalmstra baszalmstra requested a review from Wodann June 12, 2020 10:03
@baszalmstra baszalmstra self-assigned this Jun 12, 2020
@codecov
Copy link

codecov bot commented Jun 12, 2020

Codecov Report

Merging #223 into master will decrease coverage by 0.05%.
The diff coverage is 62.32%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
crates/mun/src/main.rs 0.00% <0.00%> (ø)
crates/mun_compiler_daemon/src/lib.rs 0.00% <0.00%> (ø)
crates/mun/src/lib.rs 52.06% <52.06%> (ø)
crates/mun_compiler/src/driver.rs 55.69% <53.67%> (-23.15%) ⬇️
crates/mun_hir/src/input.rs 50.00% <60.00%> (+10.00%) ⬆️
crates/mun_compiler/src/lib.rs 67.50% <78.78%> (+67.50%) ⬆️
crates/mun_project/src/manifest/toml.rs 90.00% <90.00%> (ø)
crates/mun_project/src/package.rs 92.00% <92.00%> (ø)
crates/mun_project/src/manifest.rs 96.55% <96.55%> (ø)
crates/mun/tests/integration.rs 100.00% <100.00%> (ø)
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72d2dd8...38e497b. Read the comment docs.

@Wodann Wodann added the type: feat New feature or request label Jun 13, 2020
@Wodann Wodann added this to the Mun v0.3.0 milestone Jun 13, 2020
Copy link
Collaborator

@Wodann Wodann left a 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 🙂

crates/mun/src/main.rs Outdated Show resolved Hide resolved
crates/mun/src/main.rs Outdated Show resolved Hide resolved
crates/mun_compiler/src/driver.rs Outdated Show resolved Hide resolved
crates/mun_compiler/src/driver.rs Show resolved Hide resolved
crates/mun_compiler/src/driver.rs Outdated Show resolved Hide resolved
crates/mun_compiler/src/driver.rs Outdated Show resolved Hide resolved
crates/mun_compiler/src/driver.rs Outdated Show resolved Hide resolved
crates/mun_compiler/src/driver.rs Outdated Show resolved Hide resolved
crates/mun_project/Cargo.toml Outdated Show resolved Hide resolved
@baszalmstra
Copy link
Collaborator Author

baszalmstra commented Jun 14, 2020

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?

@Wodann
Copy link
Collaborator

Wodann commented Jun 14, 2020

Yes you are right. Do you suggest I add the test flow I described above?

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:

  • create file
  • edit file
  • rename file
  • delete file

@baszalmstra baszalmstra force-pushed the feature/project branch 2 times, most recently from 0b391ed to 5b9103e Compare June 19, 2020 10:42
@baszalmstra
Copy link
Collaborator Author

Ive added some more tests. The build part of the executable is now tested by compiling and running a simple mun project. Codecov doesnt report this correctly though..

The crates mun_project currently has 90%+ coverage.

Copy link
Collaborator

@Wodann Wodann left a 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! 👍

crates/mun_runtime/src/lib.rs Outdated Show resolved Hide resolved
crates/mun_runtime/src/macros.rs Outdated Show resolved Hide resolved
@baszalmstra baszalmstra merged commit 8a86e66 into mun-lang:master Jun 24, 2020
@baszalmstra baszalmstra mentioned this pull request Jun 29, 2020
3 tasks
@baszalmstra
Copy link
Collaborator Author

Relates to #203

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feat New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants