Skip to content

no errors when processing conf file entries with comments #14

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.

Sign up for GitHub

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

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

no errors when processing conf file entries with comments #14

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions mainwindow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -782,8 +782,10 @@ void MainWindow::askAndLoadConfFile()
bool MainWindow::loadConfFile(const QString filename)
{
QFile confFile (this);
QString currentLine;
QString fullCurrentLine; // contains possible comments
QString currentLine; // stripped line without comments
int equalPosition;
int commentPosition; // position of comment character #
QString key;
QString value;
bool result;
Expand All @@ -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.

fullCurrentLine.remove(' ');

if( currentLine.startsWith("#@#") )
if( fullCurrentLine.startsWith("#@#") )
{
currentLine.remove(0, 3); //A line starting with extra_options_prefix is valid
fullCurrentLine.remove(0, 3); //A line starting with extra_options_prefix is valid
enabledOption = false;
}

if( !currentLine.startsWith('#') && currentLine.contains('=') ) //Ignore comments and lines without '='
if( !fullCurrentLine.startsWith('#') && fullCurrentLine.contains('=') ) //Ignore comments and lines without '='
{
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 •
edited
Loading

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 •
edited
Loading

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.

else
currentLine=fullCurrentLine;
equalPosition = currentLine.indexOf('=');
key = currentLine.left( equalPosition );
value = currentLine.right( currentLine.size() - equalPosition - 1 );
value.chop(1); //Chop the last character ('/n')
value.chop(1); // Chop the last character ('/n')

if (key == "drill-front")
{
Expand Down