Create function to handle relative paths #1307
Conversation
There was a problem hiding this comment.
Looks fine to me, just the request for a test.
Out of curiosity any reason why you didn't use pathlib? (Not requesting you do, this works 👍)
|
Although there's only really one or two places to use it here, +1 to |
|
I've looked a bit at pathlib, and I'm not seeing a very clean way to do this. I could do something like Path(file), then use other pathlib function to append "../" then axl_path. That doesn't seem cleaner though... |
| @@ -45,7 +46,7 @@ def setUpClass(cls): | |||
| def test_big_tournaments(self, tournament): | |||
| """A test to check that tournament runs with a sample of non-cheating | |||
| strategies.""" | |||
| filename = "test_outputs/test_tournament.csv" | |||
| filename = axl_filename("test_outputs/test_tournament.csv") | |||
There was a problem hiding this comment.
Also with pathlib, here you could do something like
filename = axl_filename() / "test_outputs" / "test_tournament.csv
assuming that axl_filename with no arguments just returns the right directory. This might be better for platform independence, since / and \ are used on different platforms.
There was a problem hiding this comment.
I like this idea to get it platform independent. But actually the cast to string needs to happen before applying the / operator. So this format wouldn't work.
What I did instead was take multiple arguments so that you could do:
axl_filename("test_outputs", "test_tournament.csv")
Then the function would apply the / operator inside.
|
This needs to be rebased on to master after #1308 got merged. |
|
LGTM as well, please rebase so we can merge |
I merged recently. I'm seeing "This branch has no conflicts with the base branch". Is there something more I should do? |
|
(Apologies if you know all this:) GitHub allows you to merge PRs in different ways. If we went for a merge commit, the icon looks like:
If we wanted to rebase (which would be tidier), the icon looks like this:
Usually that defaults to the last one you've used. We don't really have a system/convention (I'd suggest mainly due to my poor knowledge of version control at the start of this project that means our history is pretty messy) but from @marcharper's comment I assumed he wanted a rebase merge :) (In which case if you rebase on master and force push we'll be able to click that.) |
|
Either way is fine with me |
A lot of the file reads and writes are causing me problems because my IDE tries to run logic from the location where the file is located, and the code is written to only be run from the top-level directory. I could see a new user being thrown by this too.
This change makes paths a little more robust, by accounting for the path the user is actually in.