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

Interpret paths in diesel.toml as relative to the project root #2069

Merged
merged 8 commits into from
Mar 6, 2020

Conversation

sgrif
Copy link
Member

@sgrif sgrif commented May 16, 2019

I went ahead and also changed the search for project root to look for
diesel.toml first, which should improve our behavior in workspaces.

Fixes #2057.

@sgrif sgrif requested a review from a team May 16, 2019 19:42
Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

Diesel CLI test has been failed in this assertion. It seems that the path was changed.

assert!(
result
.stderr()
.contains("Command would result in changes to src/my_schema.rs"),
"Unexpected stderr {}",
result.stderr()
);

Copy link

@AlterionX AlterionX left a comment

Choose a reason for hiding this comment

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

Just a couple of nits.

before looking for `Cargo.toml`.

* Any relative paths in `diesel.toml` will now be treated as relative to the
project root (the directory containing either `diesel.toml` or `Cargo.toml`).

Choose a reason for hiding this comment

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

I think that this clarification makes it a little more confusing. It might be more effective to state in the previous paragraph what the project root is, then just reuse the term, especially since there can be multiple Cargo.toml files (which this PR is trying to fix anyways).

CargoTomlNotFound => {
"Unable to find Cargo.toml in this directory or any parent directories."
ProjectRootNotFound => {
"Unable to find diesel.toml or Cargo.toml in this directory or any parent directories."

Choose a reason for hiding this comment

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

It may be useful to indicate what "this" directory is. I don't know much about efficiency if you include a format! here, though. That would entail passing in either the url into the error, or something similar, which might cause enum size differences? But I'm not sure.

@weiznich weiznich added this to the 2.0 milestone Jan 22, 2020
@sezna
Copy link

sezna commented Feb 12, 2020

I was looking on the pipeline to see what failed and Azure says the build wasn't found -- does anybody know why it failed the pipeline?

@weiznich weiznich force-pushed the sg-relative-config-paths branch 2 times, most recently from 954ddd3 to 46fe6dc Compare March 4, 2020 10:07
sgrif and others added 6 commits March 4, 2020 16:02
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.

Running diesel migrations generates schema.rs at path that depends on cwd
5 participants