Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom filesystem support #8

Merged
merged 3 commits into from
Oct 22, 2018
Merged

Conversation

fboranek
Copy link
Contributor

@fboranek fboranek commented Sep 17, 2018

MR is ready to review these features:

  • fixed version of Dano MR Add FilesystemInterface_t support
  • added support for Python as well
  • moved handling relative / absolute path from several places into file system implementation
  • added tests

@fboranek fboranek force-pushed the dano-filesystem branch 4 times, most recently from 375f29e to 95717f2 Compare September 19, 2018 23:14
@burlog
Copy link
Contributor

burlog commented Oct 15, 2018

Hi,

I've some general objections to your pull request.

  • it contains plenty coding style changes that's not needed - remove them please,
  • you added one more testing framework - it's rather general objection, there is Google Test tests now and you should use it. I know that they are disabled but we wan't do things better, don't we? But on other side for Teng3 I rewrite all tests to Catch2 in BDD style. It may be nice if you rewrite test to Catch2 either but I will be satisfied if you left them in Boost Test.
  • you made Teng depended on Boost library - remove it please (use std::hash instead).

I've some other minor objections also which we can discuss after you fix all of I mentioned above.

@fboranek
Copy link
Contributor Author

  • it contains plenty coding style changes that's not needed - remove them please,

done

  • you made Teng depended on Boost library - remove it please (use std::hash instead).

Yes, I added the dependency on boost, but it just a compile dependency. std::hash is since C++11 and it doesn't occur to me that the project uses C++11. I enable C++14 just on tests, actually, the tests are compiled and run only if C++14 is available. If you are ok with an upgrade to C++11 a can use the standard library.

  • you added one more testing framework - it's rather general objection, there is Google Test tests now and you should use it. I know that they are disabled but we wan't do things better, don't we? But on other side for Teng3 I rewrite all tests to Catch2 in BDD style. It may be nice if you rewrite test to Catch2 either but I will be satisfied if you left them in Boost Test.

It looks very similar. It seems to me it better rewrite it in your branch. I will gladly do that. For now let's stick with boost.

@burlog
Copy link
Contributor

burlog commented Oct 18, 2018

Let's enable C++11. It should be ok.

@fboranek
Copy link
Contributor Author

Done.

@burlog burlog merged commit 9351b09 into seznam:master Oct 22, 2018
@fboranek fboranek deleted the dano-filesystem branch October 22, 2018 18:46
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.

2 participants