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

Fix test UAIFileTest.preamble failure for some locales #3802

Merged
merged 1 commit into from May 3, 2017

Conversation

luisffranca
Copy link
Contributor

The unit test UAIFileTest.preamble failed for locales with other decimal delimiters, such as "pt_BR.utf8". Some functions, like strtod(), use the "C" locale and the test failed when they were called with a different one.

This is fixed by using the "C" locale in CUAIFile::get_vector().

@luisffranca luisffranca changed the title Fix UAIFileTest.preamble failure for some locales Fix test UAIFileTest.preamble failure for some locales Apr 30, 2017
@lisitsyn
Copy link
Member

@luisffranca hey! Thanks for submitting this patch.

@vigsterkr Travis fails with a style check related to original code. Do we merge it?

@iglesias
Copy link
Collaborator

iglesias commented May 1, 2017

Should we update the clang-format style file for this? @geektoni

@iglesias iglesias self-requested a review May 1, 2017 07:17
@geektoni
Copy link
Contributor

geektoni commented May 1, 2017

@iglesias We could force clang-format to align escaped newlines as far left (or far right) as possible.
By doing this, the new code will become like this https://pastebin.com/xi5CH2GP (the output on Travis is messed up because of the terminal which cannot show correctly tabs).

@iglesias
Copy link
Collaborator

iglesias commented May 1, 2017

@geektoni I don't have strong feelings about the location of the trailing ''s; though it looks indeed nice all aligned.

Regarding the indentation between the first line with the macro signature and the following lines, I think it would be better to have everything with the same level of indentation since I believe that is more consistent with the current codebase.

@geektoni
Copy link
Contributor

geektoni commented May 1, 2017

@iglesias I don't know if there is an option to prevent indentation inside macro definitions (apart from using // clang-format off and // clang-format on which disable formatting). I will investigate a bit.

I don't think we will be able to configure clang-format to respect exactly all Shogun code conventions. There will surely be things we cannot force.

@geektoni
Copy link
Contributor

geektoni commented May 2, 2017

@iglesias I've done some research and apparently it is not possible to format #define without indentation. There are three options now. We could:

  1. Merge without caring about that style issue;
  2. Surround the #define with // clang-format off and // clang-format on instruction;
  3. Apply changes suggested by clang-format and then merge;

Since the clang-format purpose is to free devs from thinking about code formatting, I would proceed with option 3. I agree that it will change the codebase standard, but in the long run, I think it will be worth it.

@iglesias
Copy link
Collaborator

iglesias commented May 2, 2017

I agree with you, @geektoni. I think 3 is the most suitable option as well.

@geektoni
Copy link
Contributor

geektoni commented May 2, 2017

Hi @luisffranca! As you can read, one of the Travis tests failed because of some code style issues.

To fix that error please give a look to the Travis log below and you'll see the commands you have to run: https://travis-ci.org/shogun-toolbox/shogun/jobs/227279295#L524

Make sure to have clang-format-3.8 installed on your local machine or the commands won't work.

After doing that, update the PR here and you're done. 🎉

@luisffranca
Copy link
Contributor Author

Hi @geektoni! Thanks for your help.

I've updated the PR and now the tests should work!

@iglesias iglesias merged commit c2b3585 into shogun-toolbox:develop May 3, 2017
@luisffranca luisffranca deleted the bugfix/UAIFileTest branch May 7, 2017 16:32
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.

None yet

4 participants