holochain
Path string roundup
PR summary
The primary change of this PR is to convert String and &str to PathBuf and &Path when applicable to resolve #660.
This change is valuable because you may obtain a path that contains non-utf8 data and try to use it with these APIs, but if these APIs take Strings, you have to lossily convert it and you can't operate on those files.
The main non-obvious change as a result of this effort are changes like:
format!("using path: \'{}\'", some_string_path)
to
format!("using path: {:?}", some_pathbuf)
these result in a change from single quotes to double quotes.
The alternative substitution is
format!("using path: \'{}\'", some_pathbuf.to_string_lossy())
this has the advantage of preserving single-quotes, but has the disadvantage of requiring an extra allocation over the more efficient Debug-impl of PathBuf.
There are existing instances in the code where the latter approach is used, it's probably worth changing all instances to use the same choice. Which one seems better?
The other change that this PR makes is to change all &PathBufs in signatures to &Paths. This is liking changing &String to &str, no downside. Read further in the first commit message.
testing/benchmarking notes
The test suite fails for me on current develop and it also fails for me on my branch.
followups
holochain/lib3h#309
changelog
Please check one of the following, relating to the CHANGELOG-UNRELEASED.md
- [ ] this is a code change that effects some consumer (e.g. zome developers) of holochain core so it is added to the CHANGELOG-UNRELEASED.md (linked above), with the format
- summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234) - [ ] this is not a code change, or doesn't effect anyone outside holochain core development
- [x] I'm not sure which parts of the code are outward facing so I don't know
The holochain_net test failure seems spurious as it is entirely related to system time and completely self-contained. The other one I don't understand.
The
holochain_nettest failure seems spurious as it is entirely related to system time and completely self-contained. The other one I don't understand.
The others ones have to do with DocTests, Make sure even the documents are importing and using pathbuf since rustdoc also applies to syntax rules and will try to parse it :)
Under lib.rs look for bootstrap_from_config and load_config, if they are both using PathBuf make sure that PathBuf is being imported into doc scope
I believe that I implemented your feedback about the doc comments. I think that they've never run because the tests aborted early with the failures on my machine. Of the two failures now, one is a timeout that I don't understand. For the other one, I can't seem to scroll down in the log to find how it fails :confused: .
k about the doc comments. I think that they've never run because the tests aborted early with the failures on my machine. Of the two failures now, one is
yeah that seems like an issue at the Circle CI part. I would change something very small just to trigger it again
It's been re-run and now I can see the failure log but there's a lot of output and it's not clear to me where the failure is. Thanks for your help by the way :)
I'm happy to fix it once I understand the failure.
It looks th
It's been re-run and now I can see the failure log but there's a lot of output and it's not clear to me where the failure is. Thanks for your help by the way :)
I'm happy to fix it once I understand the failure.
It looks like the app-spec failed to run again :( dont know what is going on at the CI
also no problem!
I have a full log. I'll paste it. https://gist.github.com/timotree3/3dfbc59a96a00e0ba6ab75c60b14a905
I have a full log. I'll paste it. https://gist.github.com/timotree3/3dfbc59a96a00e0ba6ab75c60b14a905
will take a look at this!
I have a full log. I'll paste it. https://gist.github.com/timotree3/3dfbc59a96a00e0ba6ab75c60b14a905
Looks like it was flaky test, after the conflict is resolved and we get one more review we could approve this imo
Awesome! I'll get around to resolving these conflicts in the next few days..
@timotree3 would you mind checking I fixed those conflicts correctly and didn't undo any of your work
Sure! That's a huge list of changes on that merge commit. I'll try on git CLI soon and see if that shows only the resolved conflicts.
The change list was better on CLI. Those resolutions LGTM. Thanks @willemolding!
Hey @timotree3 can you try merging develop in now that a few monster PRs went in? I took at a look at the diff and it's actually not looking too bad, just a few of your changes got clobbered. I would clean it up but it's on your fork. It would be cool to merge this in as soon as the conflicts are resolved.
Sure! I'll do that within the next several hours. By the way, I set it up so maintainers can push to my branch. I'm glad this is making progress!!
Edit: Oops didn't mean to close and comment
Thanks for doing this! Also @timotree3 if I was wrong about the merge not being too bad, and it actually is really bad, let us know. Running fmt on your branch might help if so.
I believe all the failing tests are caused by the race condition that is fixed in #1782
I think the reason all my PRs aren't making progress is because I don't have the ability to request reviews. @maackle, since you approved this, would you like to request someone else's review so that this can make progress? I'll resolve the merge conflicts.
As a side note: I don't understand why we need two approvals to merge something.