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

no errors when processing conf file entries with comments #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AlexeyGusev
Copy link

fix for #13

Copy link
Contributor

@eyal0 eyal0 left a comment

Choose a reason for hiding this comment

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

Thanks for your PR!

{
commentPosition = fullCurrentLine.indexOf('#');
if (commentPosition!=-1)
currentLine = fullCurrentLine.left( commentPosition+1 );
Copy link
Contributor

Choose a reason for hiding this comment

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

Why +1 here? Below there is a search for the character = and the following line does not use +1

Copy link
Author

Choose a reason for hiding this comment

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

because you are doing value.chop(1) several lines later, which originally throws \n away. To be orthogonal to that, I add the trailing trail symbol to the value variable contents (to compensate for missing \n, which we have thrown away together with the comment), otherwise a bit too much chopping happens.

Copy link
Contributor

@eyal0 eyal0 Dec 3, 2018

Choose a reason for hiding this comment

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

I see. That's tricky. Maybe it would be best to do the chop on the fullCurrentLine earlier and then you don't have to do this? You could do it before calling .indexOf(#)?

Even if you don't do this, the comment in chop line below is now wrong.

Copy link
Author

@AlexeyGusev AlexeyGusev Dec 3, 2018

Choose a reason for hiding this comment

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

fail to see why this is tricky - I tested and it works. Chopping first and then doing the rest will work, sure. Up to you to decide which way you go, but I see no problem with proposed solution, really. Yeah, the comment should say smth like «chop the trailing \n or #» in this case. Or just «chop the trailing character»

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean that it's tricky because it's not obvious in the reading why I would want to keep the # around. Only after spending more time looking does the answer appear. Code that is not obvious is harder to read.

I'm not very particular about it so if you don't want to change it, that's okay.

@@ -797,21 +799,26 @@ bool MainWindow::loadConfFile(const QString filename)
while( !confFile.atEnd() )
{
enabledOption = true;
currentLine = confFile.readLine();
currentLine.remove(' ');
fullCurrentLine = confFile.readLine();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there even need for this rename? I think that you could keep the original name and there would be no problem.

Copy link
Author

Choose a reason for hiding this comment

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

to keep all the rest of the code below intact.
You read a line from a file into fullCurrentLine, then either just copy contents into currentLine if there is no comment on that line or strip the comment away and copy remaining, useful part into currentLine. Otherwise you would have to do currentLine = currentLine.left( commentPosition+1) ­not sure it's a good idea. At least it simplified debugging and testing while I was writing this code.

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