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: implement --verbose flag on ion template inspect cmd #21

Merged
merged 41 commits into from
Feb 22, 2023
Merged

Feat: implement --verbose flag on ion template inspect cmd #21

merged 41 commits into from
Feb 22, 2023

Conversation

diversable
Copy link
Contributor

I had a little extra time over the last couple of days, so I thought I'd finish this off.

I also included an update to the README / docs, just to flesh it out a bit until you have the time to fill out the Readme a little more with your own words.

@diversable
Copy link
Contributor Author

This commit should fully resolve issue: #9 (comment)

diversable and others added 9 commits February 18, 2023 10:59
modified grcov action by commenting out `actions-rs/install` - as the doc comment suggests
remove rust-flags which are causing compilation of Ion-derive to fail building
@Roger-luo
Copy link
Owner

thanks, I'm trying to add codecov to the CI so probably need to wait for me a bit on fixing the CI before merging this PR then we will have a code coverage report on the unit tests.

@Roger-luo
Copy link
Owner

The CI should be fixed now, could you merge main branch to your PR? Thanks

@diversable
Copy link
Contributor Author

The CI should be fixed now, could you merge main branch to your PR? Thanks

Working on it now - just be a few minutes...

@codecov
Copy link

codecov bot commented Feb 20, 2023

Codecov Report

Base: 55.24% // Head: 55.45% // Increases project coverage by +0.21% 🎉

Coverage data is based on head (1bedf44) compared to base (d7826f5).
Patch coverage: 93.47% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #21      +/-   ##
==========================================
+ Coverage   55.24%   55.45%   +0.21%     
==========================================
  Files          74       74              
  Lines        4502     4526      +24     
==========================================
+ Hits         2487     2510      +23     
- Misses       2015     2016       +1     
Impacted Files Coverage Δ
src/ion/blueprints/components/documenter.rs 82.60% <ø> (ø)
src/ion/blueprints/components/license.rs 96.87% <ø> (ø)
src/ion/blueprints/components/project_file.rs 93.18% <ø> (ø)
src/ion/blueprints/template.rs 100.00% <ø> (ø)
src/ion/blueprints/utils.rs 60.18% <91.42%> (+2.67%) ⬆️
ion_derive/src/template.rs 100.00% <100.00%> (ø)
src/bin/ion/commands/template.rs 86.36% <100.00%> (+3.86%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

.github/workflows/grcov.yml Outdated Show resolved Hide resolved
@@ -15,12 +15,291 @@ Download tarball in the release page and extract it to your `$HOME/.julia` direc

### build from source
Copy link
Owner

Choose a reason for hiding this comment

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

it'd be nice if this can be in a separate PR, I probably will put up the website soon, so we can decide whether to have this in a different PR

@@ -22,3 +24,80 @@ pub struct Template {
pub coveralls: Option<Coveralls>,
pub github: Option<GitHub>,
}

impl fmt::Display for Template {
Copy link
Owner

Choose a reason for hiding this comment

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

this should be in derive macro for Template, so things can be kept extensible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the help!

The latest commit should resolve these issues, I believe.

self.name, self.description
)?;

if let Some(repo) = &self.repo {
Copy link
Owner

Choose a reason for hiding this comment

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

you need to generate the names instead of manually writing them, like how I generate other methods

Copy link
Contributor Author

@diversable diversable Feb 20, 2023

Choose a reason for hiding this comment

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

this one might take a little while -- I have zero experience writing Rust macros (I know how to use them, just not how to write them).

But I'm reading up on it. The resources are a little thin, but I'll get it eventually...

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, perhaps we should just do the debug printing, so that you don't need to implement this now

@Roger-luo
Copy link
Owner

actually, why don't we just use the debug print? It seems you are implementing something similar anyway?

@diversable
Copy link
Contributor Author

diversable commented Feb 20, 2023

If you prefer the debug print output, I can certainly do it that way. However, I did actually try the straight debug print option first, before the current implementation, and found that I didn't have enough context to understand what that debug output actually meant.

I realize that improvements to the display output could be made -- maybe printing out the info in tabular form would be better, or something like that? -- I just don't have enough experience with the templating sub-system right now to be able to design a better output.

That's one of the reasons I listed some issues on the templates repo: I think once I make 1 or 2 of my own templates, I think I'll have a better handle on what needs to be displayed to make the output truly informative for future users. Then I can come back around and redesign this output (or at least add color / additional structure).

Thoughts?

Would you prefer just to do the debug output right now? Or are there other recommendations you have for improving the intelligibility of the output?

Thx.

@Roger-luo
Copy link
Owner

Would you prefer just to do the debug output right now? Or are there other recommendations you have for improving the intelligibility of the output?

Yeah I think maybe let's just do the debug print for now since it's way more simpler and also verbose enough, especially it doesn't require us changing the procedure macro, we can think better printings in the future.

@diversable
Copy link
Contributor Author

Would you prefer just to do the debug output right now? Or are there other recommendations you have for improving the intelligibility of the output?

Yeah I think maybe let's just do the debug print for now since it's way more simpler and also verbose enough, especially it doesn't require us changing the procedure macro, we can think better printings in the future.

So, I've changed to debug printing as requested, though I've left the basic code for re-implementing the Display trait on the Template and all its related sub-structs in place if we want to customize in the future.
Did you want me to get rid of that code as well, since its vestigial atm?

@diversable
Copy link
Contributor Author

Would you prefer just to do the debug output right now? Or are there other recommendations you have for improving the intelligibility of the output?

Yeah I think maybe let's just do the debug print for now since it's way more simpler and also verbose enough, especially it doesn't require us changing the procedure macro, we can think better printings in the future.

So, I've changed to debug printing as requested, though I've left the basic code for re-implementing the Display trait on the Template and all its related sub-structs in place if we want to customize in the future. Did you want me to get rid of that code as well, since its vestigial atm?

The impl Display stubs have been removed.

@Roger-luo
Copy link
Owner

Yeah I think current one looks pretty good to me! Thanks!

@Roger-luo Roger-luo merged commit c6e0e1c into Roger-luo:main Feb 22, 2023
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.

2 participants