Fix issue #535: Treat rel. paths inside config relative to the config file

[lukeshingles]

This is a quickfix for issue #535. File paths in the config file are now treated as relative to the location of the config file.

[wkerzendorf]

This is an interesting proposal. Not clear if we want to have it that way as I think most software does it relative to the CWD for these user defined config files. But what does @tardis-sn/tardis-core think?

[yeganer]

I think this change will help to keep things more organized. If I'm defining paths inside a file I expect them to be treated relative to that file, if I don't want that behavior I'd use absolute paths.

The current implementation requires me to constantly link the atom_data database to my current working directory.

[lukeshingles]

If nothing else, at least this was a good example problem for me to work on. Finally passes the tests :).

[lukeshingles]

Could include an extra setting for each input file with options to set the base path to either the config directory or CWD. Alternatively, special keywords like $CWD or $CONFIGDIR could be replaced with their values during input. Both of these can preserve the default behaviour of giving paths relative to the CWD.

[wkerzendorf]

@lukeshingles I think it's fine. If we need this later - we can add it. I think we just need to review it from the code and then merge.

[yeganer]

I think it's fine as it is. In the next month the config system might see significant changes so I wouldn't add these features now.

As for the implementation. I think it's fine. For a rewrite of the config system I'd convert everything to absolute paths but that's not necessary.

[wkerzendorf]

@tardis-sn/tardis-core are we merging this now?

[yeganer]

@wkerzendorf I'm happy to merge. If @lukeshingles wants to add a small test for that functionality it would be even better but since this is not a major change I wouldn't make it a requirement for merging.

[lukeshingles]

I'm happy to do it, although it would take me a couple of working days to have it done. What kind of test did you have in mind? Moving the working directory up a level and rewriting the config dir to match?

Also, any idea why Scrutinizer failed this commit?

[yeganer]

@lukeshingles I was thinking of the following:

• Use some config file already in the repository, get it's absolute path
• Call the config validator with the path to that file in different ways. (absolute path, local path while in that directory, local path while directory above/below)
• Test if reading in the abundance file works (for example, any other 'external' file would work, too)

Update

When I'm looking at the current implementation I think it would be useful to have a from_file builder. Since that's what we are doing 99% of the time. But there is no point in doing that at the moment since @ftsamis is in the process of rewriting the config system.

[ftsamis]

Since I plan to write/update tests for the changes I make in the configuration system and I have no extensive experience in writing tests, this would be a good chance for me to write one for the changes @lukeshingles did. May I?

[yeganer]

@ftsamis Sure, feel free to add them and make a PR to @lukeshingles 's branch.
I think it's a really good idea actually because you can make yourself familiar with py.test and use that knowledge during your own development. But don't spend too much time on the tests as they will most likely change because of the rewrite you are doing.

[ftsamis]

Please check my PR (lukeshingles#1) on @lukeshingles' branch which adds tests for this PR. This is also my first attempt to write pytest tests, so bear with me if something is not as it should be.

[wkerzendorf]

@lukeshingles @ftsamis @tardis-sn/tardis-core is that done?

[lukeshingles]

Looks good to me.

[wkerzendorf]

@lukeshingles @ftsamis well done!

[yeganer]

@tardis-sn/tardis-core Sorry I'm a bit late to the party.
I wouln't have merged this version. There is an empty commit (which could have been deleted easily) and I was about to suggest using monkeypatch to simulate the directory changes.

Pytest provides this fixture automatically to do exactly what we want.

[ftsamis]

You are right @yeganer, monkeypatch.chdir is exactly what I should have used in this one. Opened a new PR with this change: #555 , please review it.

Regarding the empty commit, I'm not sure anything can be done now on my side, except not pushing empty commits again on PR branch. I was exploring Travis, but I should have done that on a separate test branch (which I did, after this empty commit) from the beginning.

[yeganer]

@ftsamis There is no problem with pushing anything to a PR branch, but please make sure to clean up the commit history of your PR before finishing it (have a look at git rebase -i and git push -f).