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

feat(diagnostic): integrate annotate-snippets crate #91

Merged
merged 25 commits into from
Apr 13, 2020
Merged

feat(diagnostic): integrate annotate-snippets crate #91

merged 25 commits into from
Apr 13, 2020

Conversation

legendiguess
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Feb 5, 2020

Codecov Report

Merging #91 into master will increase coverage by 2.80%.
The diff coverage is 88.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #91      +/-   ##
==========================================
+ Coverage   78.86%   81.67%   +2.80%     
==========================================
  Files         141      141              
  Lines        8754     9021     +267     
==========================================
+ Hits         6904     7368     +464     
+ Misses       1850     1653     -197     
Impacted Files Coverage Δ
crates/mun/src/main.rs 0.00% <0.00%> (ø)
crates/mun_compiler/src/driver/config.rs 100.00% <ø> (ø)
crates/mun_compiler/src/lib.rs 0.00% <0.00%> (ø)
crates/mun_compiler_daemon/src/lib.rs 0.00% <0.00%> (ø)
crates/mun_syntax/src/syntax_error.rs 66.66% <50.00%> (-6.07%) ⬇️
crates/mun_compiler/src/driver/display_color.rs 72.72% <72.72%> (ø)
crates/mun_compiler/src/driver.rs 78.43% <90.00%> (+8.21%) ⬆️
crates/mun_compiler/src/diagnostics.rs 92.92% <91.91%> (+66.69%) ⬆️
crates/mun_compiler/src/diagnostics_snippets.rs 91.97% <91.97%> (ø)
crates/mun_compiler/src/annotate.rs 94.31% <94.31%> (ø)
... and 18 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 6ac6e11...d9c63c1. Read the comment docs.

crates/mun_compiler/src/diagnostics.rs Outdated Show resolved Hide resolved
crates/mun_compiler/src/diagnostics.rs Outdated Show resolved Hide resolved
crates/mun_compiler/src/diagnostics.rs Outdated Show resolved Hide resolved
crates/mun_compiler/src/diagnostics.rs Outdated Show resolved Hide resolved
crates/mun_compiler/src/diagnostics.rs Outdated Show resolved Hide resolved
crates/mun_compiler/src/diagnostics.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
@baszalmstra
Copy link
Collaborator

@legendiguess legendiguess marked this pull request as ready for review February 24, 2020 17:55
Copy link
Collaborator

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

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

Hey @legendiguess ! Thanks for adding the builders!

The only thing that is currently missing for me are tests that verify that the snippets are actually created correctly. I've seen a lot of test screenshots float by on discord, maybe you can turn them into actual regression tests?

I would like to see some tests that given an error check whether the output matches to what you would expect. You can use the snapshot crate to create tests like that. We use that a lot throughout the codebase.

crates/mun_compiler/src/annotate_snippets_builders.rs Outdated Show resolved Hide resolved
crates/mun_compiler/src/annotate_snippets_builders.rs Outdated Show resolved Hide resolved
crates/mun_compiler/src/diagnostics.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

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

Nice @legendiguess !

I added some comments here and there but the main thing I'm missing is tests for all the different types of errors. I know you create test programs for all the different types of errors. It would be great to add those as tests!

crates/mun/src/main.rs Outdated Show resolved Hide resolved
crates/mun/src/main.rs Outdated Show resolved Hide resolved
crates/mun/src/main.rs Outdated Show resolved Hide resolved
crates/mun_compiler/Cargo.toml Outdated Show resolved Hide resolved
crates/mun_compiler/src/diagnostics.rs Outdated Show resolved Hide resolved
crates/mun_compiler/src/diagnostics.rs Show resolved Hide resolved
crates/mun_compiler/src/diagnostics.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/config.rs Outdated Show resolved Hide resolved
@baszalmstra baszalmstra requested a review from Wodann April 8, 2020 13:43
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, a very good PR. You improved test coverage a lot, which is important to verify that your code works! 👍

Some comments for next time:

  • You put a lot of work into a single PR. As a result, it can seem that there are a lot of things "wrong" or "to fix" after reviewing. This can be a bit demotivating, I think. It also makes it harder for us to review, because there is so much to look at.
  • There is still a lot of code duplication.

When writing code, try to find the right balance between code duplication and readability. E.g. sometimes you can write something with few words, but it is hard to understand what is going on. Other times there is a lot of code duplication which requires changes in many different places. If someone forgets one place, it can result in errors. So both can be dangerous, hence try to find a good balance 😄

I also think it will help you learn more if you make smaller PRs, because you go through the cycle of programming -> review -> learn -> improve -> review -> finish more often. The tips you'll receive during each review will be fewer, making it easier to learn on those topics.

crates/mun_compiler/src/annotate_snippets_builders.rs Outdated Show resolved Hide resolved
crates/mun_compiler/src/annotate_snippets_builders.rs Outdated Show resolved Hide resolved
crates/mun_compiler/src/annotate_snippets_builders.rs Outdated Show resolved Hide resolved
crates/mun_compiler/src/annotate_snippets_builders.rs Outdated Show resolved Hide resolved
crates/mun_compiler/src/annotate_snippets_builders.rs Outdated Show resolved Hide resolved
crates/mun_hir/src/line_index.rs Outdated Show resolved Hide resolved
crates/mun_hir/src/line_index.rs Outdated Show resolved Hide resolved
crates/mun_syntax/src/ast/extensions.rs Outdated Show resolved Hide resolved
crates/mun_syntax/src/ast/extensions.rs Outdated Show resolved Hide resolved
crates/mun_syntax/src/ast/extensions.rs Outdated Show resolved Hide resolved
@Wodann
Copy link
Collaborator

Wodann commented Apr 11, 2020

Just needs a squash before this can be merged. I will do that

@Wodann
Copy link
Collaborator

Wodann commented Apr 13, 2020

Cleaned up history and rebased on master. Waiting for CI to succeed, then I will merge.

@Wodann
Copy link
Collaborator

Wodann commented Apr 13, 2020

Removed changes to submodule hashes from respective commits, because this PR should really not be touching those. This was probably erroneously committed as a result of failing to git submodule update

@Wodann Wodann merged commit 857fdd5 into mun-lang:master Apr 13, 2020
@baszalmstra baszalmstra mentioned this pull request Apr 13, 2020
5 tasks
@Wodann Wodann added this to the Mun v0.2 milestone May 14, 2020
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.

3 participants