feat(#41): latex.rs, puzzles #47
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #47 +/- ##
==========================================
+ Coverage 53.33% 58.82% +5.49%
==========================================
Files 4 5 +1
Lines 15 17 +2
==========================================
+ Hits 8 10 +2
Misses 7 7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
@h1alexbel take a look at my comments below, please. Also I'm not sure that this PR is closing the tickets, should be use ref in description instead of closing or refactor PR to fully close the issue, wdyt?
server/src/report/latex.rs
Outdated
| /// ``` | ||
| pub fn template(path: Option<&str>) -> String { | ||
| return fs::read_to_string(Path::new(path.unwrap_or("resources/report.tex"))) | ||
| .unwrap(); |
There was a problem hiding this comment.
@h1alexbel it's not the best practice to use #unwrap, my suggestion is to setup clippy to exclude usage of #unwrap
There was a problem hiding this comment.
@l3r8yJ Should I use match instead?
@l3r8yJ Let me explain then. #41 is quite abstract and we had a conversation in it about additional flags and so on. I decided to "kick off" this ticket here, and decompose future work into more manageable and easy-to-go tickets. That's why I committed a few related puzzles as well. WDYT? |
|
need to adjust puzzles according to this comment: #41 (comment) |
|
@h1alexbel can you refactor puzzles and request again? |
|
@l3r8yJ yes |
|
@l3r8yJ the branch is ready |
server/src/report/latex.rs
Outdated
| /// let content = template(None); | ||
| /// print!("{content}") | ||
| /// ``` | ||
| pub fn template(path: Option<&str>) -> String { |
There was a problem hiding this comment.
@h1alexbel i think in this case path should be required, you can add it to Args and set default value if nothing was provided to --report argument; txt would be fine for default value
There was a problem hiding this comment.
@l3r8yJ yes, this function will be called only if cli parser will find non-empty --report option.
There was a problem hiding this comment.
@h1alexbel can you add the puzzle or implementation?
There was a problem hiding this comment.
@l3r8yJ it's already added as puzzle in cli/src/args.rs
|
Excellent review Analyzed with |
|
@rultor merge |
@h1alexbel Can't merge it. Some CI checks were failed. Apparently, the pull request is not ready to be merged since it has some problems. Please, fix them first. |
|
@rultor merge |
@h1alexbel Can't merge it. Some CI checks were failed. Apparently, the pull request is not ready to be merged since it has some problems. Please, fix them first. |
|
@rultor status |
|
@rultor merge |
@h1alexbel This is what's going on here:
More information about Rultor commands you can get here. |
@h1alexbel Can't merge it. Some CI checks were failed. Apparently, the pull request is not ready to be merged since it has some problems. Please, fix them first. |
|
@rultor merge |
@h1alexbel Can't merge it. Some CI checks were failed. Apparently, the pull request is not ready to be merged since it has some problems. Please, fix them first. |
|
@rultor stop |
|
@rultor merge |
@h1alexbel OK, I'll try to stop current task. |
@h1alexbel Can't merge it. Some CI checks were failed. Apparently, the pull request is not ready to be merged since it has some problems. Please, fix them first. |
@h1alexbel Sorry, I failed to stop the previous command, however it has the following result: Oops, I failed. You can see the full log here (spent 14s) |
@l3r8yJ take a look, please
closes #41
PR-Codex overview
This PR adds a new
reportmodule withlatex.rsfor generating reports in LaTeX format. It also includes a function to read a LaTeX template file and a test for template content comparison.Detailed summary
reportmodule withlatex.rsfor report generation