cliqz-oss / keyvi

Keyvi - a key value index that powers Cliqz search engine. It is an in-memory FST-based data structure highly optimized for size and lookup performance.
https://cliqz.com
Apache License 2.0
179 stars 38 forks source link

add Clion IDE configuration, move CMakeLists.txt into keyvi folder #219

Closed hendrikmuhs closed 7 years ago

hendrikmuhs commented 7 years ago

@narekgharibyan

Can you check whether this works for you?

Some minor adjustments for e.g. python might be needed.

Note: They say 'workspace.xml' is not to be put into VCS, but that's actually not true, it is required to get the targets right. So this file is a manually cleaned up version just to get the targets in.

Please try with a fresh clone, follow the instructions in Readme and see whether Clion works for you.

narekgharibyan commented 7 years ago

@hendrikmuhs I suggest from this PR to take only debug mapping fix and leave the rest as it is. Personally I find asking people copying .idea directories so some locations a bit inconvenient.

hendrikmuhs commented 7 years ago

@narekgharibyan I don't mind. I think it is a good idea to have some documentation about how to get started, setting up the IDE is part of that. But agreed, there is no need to have the config files there, it's probably better to provide it as documentation.

I definitely do not want to put any IDE setting files into the repo along with the source code, that's why contrib was the choice. contrib is also a good place for placing potentially unmaintained stuff in.

Anyway, just waiting for travis now and next checking whether we can get rid of the coverage sed commands as discussed.

Then I will remove the Clion config files and create some documentation instead, I think the usual way is to create a CONTRIBUTING.md file.

What do you think?

narekgharibyan commented 7 years ago

I have no strong feelings there, I'm fine even without them. But let's keep current location of CMakeLists.txt

hendrikmuhs commented 7 years ago

In my opinion CMakeLists.txt should not be in the root, but in the keyvi folder. The only reason for having it in the root was problems with Clion, these problems are solved.

Anyway, if that's the only disagreement, than in the interest of the project I leave it there.

narekgharibyan commented 7 years ago

@hendrikmuhs Agree! Just I do have a local fixes, regarding to applying https://github.com/cliqz-oss/keyvi/issues/214 to 0.2 branch. And wanted to move it afterwards. But not a problem, I'll redo my changes! Feel free to move it now.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-10.2%) to 76.451% when pulling ea77295ed995f1cd679c56da2c52ac005a45b774 on hendrikmuhs:clion-ide into f641487741ed758f61325e68512bd91dd4786f79 on cliqz-oss:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 86.698% when pulling 86a1eaea747b3af4eb5efe33a0d1fdee4395c751 on hendrikmuhs:clion-ide into f641487741ed758f61325e68512bd91dd4786f79 on cliqz-oss:master.

narekgharibyan commented 7 years ago

@hendrikmuhs can we close this PR ?

hendrikmuhs commented 7 years ago

yes!