Skip to content

Conversation

@mfeurer
Copy link
Collaborator

@mfeurer mfeurer commented Apr 4, 2018

No description provided.

@mfeurer mfeurer requested a review from janvanrijn April 4, 2018 10:02
Copy link
Member

@janvanrijn janvanrijn left a comment

Choose a reason for hiding this comment

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

Up to discretion of @mfeurer

  • we don't need to create cache directory structure already in set_cache_dir() fn
  • for this reason the config object doesn't need getters and setters

I will approve once the unit tests succeed

@mfeurer
Copy link
Collaborator Author

mfeurer commented Apr 4, 2018

This now also partially solves #396. However, not as nicely as expected.

Copy link
Member

@janvanrijn janvanrijn left a comment

Choose a reason for hiding this comment

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

Approved. But Please address my comments :)

def test_get_setup(self):
# no setups in default test server
openml.config.server = 'https://www.openml.org/api/v1/xml/'
openml.config.set_server_url('https://www.openml.org/api/v1/xml/')
Copy link
Member

Choose a reason for hiding this comment

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

why is this still a fn?

def tearDown(self):
os.chdir(self.cwd)
shutil.rmtree(self.workdir)
for i in range(10):
Copy link
Member

Choose a reason for hiding this comment

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

make 10 a variable?

@ArlindKadra
Copy link
Member

ArlindKadra commented Apr 10, 2018

@mfeurer @janvanrijn If you like the current implementation, you can merge the pull the request. Build is passing.

@mfeurer
Copy link
Collaborator Author

mfeurer commented Apr 11, 2018

Thanks for the fix @ArlindKadra. Could you please describe which change was actually necessary to fix it? Also, could you please explain the reasoning for changing the static_cache_dir variable?

@mfeurer mfeurer merged commit 87ad7b1 into develop Apr 11, 2018
@mfeurer mfeurer deleted the fix/#397 branch April 11, 2018 08:30
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.

4 participants