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

Generate helpful errors from dotenv! macro #53

Closed
wants to merge 2 commits into from
Closed

Generate helpful errors from dotenv! macro #53

wants to merge 2 commits into from

Conversation

Plecra
Copy link
Contributor

@Plecra Plecra commented Jun 25, 2020

An implementation for #9

@codecov
Copy link

codecov bot commented Jun 25, 2020

Codecov Report

Merging #53 into master will decrease coverage by 2.43%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
- Coverage   86.77%   84.33%   -2.44%     
==========================================
  Files           7        7              
  Lines         242      249       +7     
==========================================
  Hits          210      210              
- Misses         32       39       +7     
Impacted Files Coverage Δ
dotenv_codegen_implementation/src/lib.rs 0.00% <0.00%> (ø)

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 8c2ba38...31e48e8. Read the comment docs.

@Plecra
Copy link
Contributor Author

Plecra commented Jun 25, 2020

What would you like me to do about those regressions? I'm pretty sure I haven't removed any tests, and the only changes I made were in the macro implementation

@ZoeyR
Copy link
Contributor

ZoeyR commented Jun 25, 2020

I'll look into it, but its been long enough since code coverage was run, I wouldn't be surprised if its just something going wrong with the codecov tool

@Plecra
Copy link
Contributor Author

Plecra commented Jun 26, 2020

Ooh, I can actually understand the report now. It's identified the couple error cases that I introduced but didn't test. I don't think it's worth putting together a compile error test harness for those couple cases though...

@ZoeyR
Copy link
Contributor

ZoeyR commented Jun 26, 2020

@Plecra its also in codegen, which can't really be code coverage tested.

Err(VarError::NotPresent) | Err(VarError::NotUnicode(_)) => panic!("{}", err_msg),
match env::var(var_name.value()) {
Ok(val) => Ok(quote!(#val).into()),
Err(VarError::NotPresent) | Err(VarError::NotUnicode(_)) => Err(syn::Error::new(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should break up this match arm now that we're producing more descriptive error messages. NotUnicode doesn't match with environment variable {} not defined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Taking a second look at this, I want to mirror the behavior of std::env!, which actually doesn't differentiate between the two errors. Not that that means we can't though

@Plecra
Copy link
Contributor Author

Plecra commented Jul 14, 2020

Continued in #58

@Plecra Plecra closed this Jul 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.

2 participants