Skip to content

Commit

Permalink
xmlLoadFile print error (#174)
Browse files Browse the repository at this point in the history
  • Loading branch information
@CrosRoad95 @ccw808
CrosRoad95 authored and ccw808 committed Nov 29, 2017
1 parent 2d164a0 commit ee35900
Showing 1 changed file with 3 additions and 0 deletions.
3 changes: 3 additions & 0 deletions Shared/mods/deathmatch/logic/luadefs/CLuaXMLDefs.cpp
View file
Expand Up @@ -138,6 +138,7 @@ int CLuaXMLDefs::xmlLoadFile ( lua_State* luaVM )

// Grab our resource
CLuaMain* pLuaMain = m_pLuaManager->GetVirtualMachine ( luaVM );
SString strError = "";
if ( pLuaMain )
{
SString strFileInput;
Expand Down Expand Up @@ -184,6 +185,8 @@ int CLuaXMLDefs::xmlLoadFile ( lua_State* luaVM )
}
}

xmlFile->GetLastError ( strError );
argStream.SetCustomError(strError, SString("Unable to read XML file %s", strFileInput.c_str()));
// Destroy it if we failed
pLuaMain->DestroyXML ( xmlFile );
}
Expand Down

9 comments on commit ee35900

@ArranTuna
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some players are now getting this debug: "Unable to read XML file @images.xml @ 'xmlLoadFile' [Line 0: Failed to open file]" is that because the file doesn't exist? But there is no function to check if an XML file exists, so this debug is actually just annoying rather than helpful.

@sbx320
Copy link
Member

@sbx320 sbx320 commented on ee35900 Nov 29, 2017

Choose a reason for hiding this comment

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

But there is no function to check if an XML file exists

fileExists should work

@CrosRoad95
Copy link
Contributor Author

Choose a reason for hiding this comment

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

check my example script. There are line 8 xmlLoadFile("meta.xml") meta.xml exists, 0 error/warning. 1-7 line print error

@ccw808
Copy link
Member

@ccw808 ccw808 commented on ee35900 Nov 29, 2017

Choose a reason for hiding this comment

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

This commit has been reverted. Please make a new pull request which excludes a warning for non existent files.

@jushar
Copy link
Contributor

@jushar jushar commented on ee35900 Nov 30, 2017

Choose a reason for hiding this comment

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

What do you think about just logging a warning instead of throwing an error?

@ArranTuna
Copy link
Collaborator

Choose a reason for hiding this comment

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

It was a debug warning.

@Dezash
Copy link
Contributor

@Dezash Dezash commented on ee35900 Nov 30, 2017

Choose a reason for hiding this comment

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

@ArranTuna why would you try to load a non existent file in the first place?

@ArranTuna
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because whoever wrote the script didn't realise or think they could use fileExists to see if it existed which you can't blame them for if xmlLoadFile simply returned false if it didn't exist because then they could use that fact to determine whether the XML file existed or not.

@Dezash
Copy link
Contributor

@Dezash Dezash commented on ee35900 Nov 30, 2017

Choose a reason for hiding this comment

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

@ArranTuna that's what debug messages are for, they notify the programmer about a potentially bad coding practice, defects or problems. A warning message won't break any compatibility since it doesn't stop the execution of the script and xmlLoadFile will still return false. It will help scripters improve their code and/or track down problems. Functions like dxCreateTexture already work this way so I don't see why xmlLoadFile should be different.

Please sign in to comment.