Issue 336 2

These patches consolidate the parse of command line options to ConfigManager.
In addition, it also parses a configuration file.
> Do you prefere to ConfigManager::Impl::loadConfFile() rather than
ConfigManager::loadConfFile()?

Current ConfigManager::Impl::loadConfFile()seems to be little unnatural
because it doesn't reads/writes member in Impl. However, we will add code
that reads key/values from the
configuration file and sets them to members in Impl. So I think that being
it in Impl
is not bad.



2014-08-12 16:25 GMT+09:00 Kouhei Sutou <notifications@github.com>:

> In server/src/ConfigManager.cc:
>
> > +		GKeyFile *keyFile = g_key_file_new();
> > +		if (!keyFile) {
> > +			MLPL_CRIT("Failed to call g_key_file_new().\n");
> > +			return false;
> > +		}
> > +		Reaper<GKeyFile> keyFileReaper(keyFile, g_key_file_unref);
> > +
> > +		GError *error = NULL;
> > +		gboolean succeeded =
> > +		  g_key_file_load_from_file(keyFile, path.c_str(),
> > +		                            G_KEY_FILE_NONE, &error);
> > +		if (succeeded)
> > +			return true;
> > +
> > +		Reaper<GError> errorFree(error, g_error_free);
> > +		if (error->domain == G_FILE_ERROR &&
>
> I wanted to say this "file not found" check is done in
> ConfigManager::loadConfFile() not in ConfigManager::Impl::loadConfFile().
>
> Do you prefere to ConfigManager::Impl::loadConfFile() rather than
> ConfigManager::loadConfFile()?
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/project-hatohol/hatohol/pull/365/files#r16098869>.
>