search

SimonEnsemble / PorousMaterials.jl

Julia package towards classical molecular modeling of nanoporous materials
GNU General Public License v3.0
51 stars 11 forks source link

Using `joinpath` for constructing path variables #75

Closed Andrew-S-Rosen closed 5 years ago

Andrew-S-Rosen commented 5 years ago

This is to address #72 regarding the use of joinpath to enable Windows-compatibility with filepaths (and to have a more robust method of constructing filepaths without worrying about slashes). I think I got them all. I have run the test suite, and everything passed on my Windows machine. Let me know if there's any issues, and I'll gladly take care of it.

ahyork commented 5 years ago

Thanks for this fix @ @arosen93 ! I just resolved a conflict in src/PorousMaterials because I had added some functions relating to the path recently. Once the travis-ci tests run everything should be good to merge this in!

SimonEnsemble commented 5 years ago

@arosen93 yes, thank you for contributing to PorousMaterials.jl! I am adding some more functionality to the grids right now: checking whether a given point in the grid is accessible through a channel.

Andrew-S-Rosen commented 5 years ago

@SimonEnsemble Of course! And that's awesome. I was a bit behind schedule and hadn't had a chance to check out the grids yet. Going to do that later today. I'll reach out if there are any issues!

ahyork commented 5 years ago

@arosen93 All the tests pass except for the path_test.jl suite. This was added after you cloned the repo but before this pr, I tried to push a commit to fix this but I didn't have access. Could you update your fork to match our master branch and then fix the three @tests in test/path_test.jl?

ahyork commented 5 years ago

The set_path_to_data(pwd()) and set_path_to_data() are function calls changing the PATH_TO_DATA variable, and shouldn't be inside of the joinpath calls. Sorry I haven't commented these tests

Andrew-S-Rosen commented 5 years ago

@ahyork Oops! Just assumed there was a * operator there. I think I got it now... maybe! Let's try again.

ahyork commented 5 years ago

@arosen93 I think this should do it! Thank you again for helping us switch everything over to joinpath!

Andrew-S-Rosen commented 5 years ago

Great! Glad to help out!

ahyork commented 5 years ago

@arosen93 Almost there! One of the three tests failed:

Expression: PorousMaterials.PATH_TO_DATA == joinpath(dirname(pathof(PorousMaterials)), "../test/data/") Evaluated: "/home/travis/build/SimonEnsemble/PorousMaterials.jl/src/../test/data" == "/home/travis/build/SimonEnsemble/PorousMaterials.jl/src/../test/data/"

I think to fix this we need to split the joinpath(dirname(pathof(PorousMaterials)),"../test/data/") into joinpath(dirname(pathof(PorousMaterials)), "..", "test", "data")

Andrew-S-Rosen commented 5 years ago

@ahyork Sounds reasonable. I wasn't sure if that one would work. I think that works in Python, so I was hoping it'd work here. Let's hope this one fixes it! Nearly there.

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 178

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/PorousMaterials.jl 3 4 75.0%
<!-- Total: 18 19 94.74% -->
Totals Coverage Status
Change from base Build 165: -0.06%
Covered Lines: 738
Relevant Lines: 845
💛 - Coveralls