-
Notifications
You must be signed in to change notification settings - Fork 5
cleanup env tests #983
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
base: matt/circleci_env_parsing
Are you sure you want to change the base?
cleanup env tests #983
Conversation
|
Merging to
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## matt/circleci_env_parsing #983 +/- ##
==========================================================
Coverage 81.02% 81.02%
==========================================================
Files 66 66
Lines 14152 14152
==========================================================
Hits 11466 11466
Misses 2686 2686 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dfrankland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| use context::env::{ | ||
| self, EnvVars, | ||
| parser::{BranchClass, CIInfo, CIPlatform, EnvParser}, | ||
| validator::{EnvValidationIssue, EnvValidationIssueSubOptimal, EnvValidationLevel}, | ||
| }; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All imports here are unused
| #[path = "env/bitbucket.rs"] | ||
| mod bitbucket; | ||
| #[path = "env/buildkite.rs"] | ||
| mod buildkite; | ||
| #[path = "env/circleci.rs"] | ||
| mod circleci; | ||
| #[path = "env/custom.rs"] | ||
| mod custom; | ||
| #[path = "env/drone.rs"] | ||
| mod drone; | ||
| #[path = "env/github.rs"] | ||
| mod github; | ||
| #[path = "env/gitlab.rs"] | ||
| mod gitlab; | ||
| #[path = "env/semaphore.rs"] | ||
| mod semaphore; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO using path is sort of a anti-pattern (since it makes it harder to reason about module hierarchy. Instead of this, I'd just prefix each of these files with env_ and keep them at the top level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok - still new to rust, I just found something that "worked" - ty

Split env.rs test file on parser boundaries, import them all in the env.rs file.