CompEvol / beast2

Bayesian Evolutionary Analysis by Sampling Trees
www.beast2.org
GNU Lesser General Public License v2.1
241 stars 84 forks source link

BEASTObject.toString() == null without set id #552

Closed walterxie closed 8 years ago

walterxie commented 8 years ago

There is a potential "null" problem, if a beast object does not has id in the xml. This is caused by BEASTObject.toString() == null.

walterxie commented 8 years ago

To solve the problem, I add auto naming code to assign the class name to id, if beastObject.getID() == null or "".

alexeid commented 8 years ago

Walter did you discuss this with any of the core team first? It seems like a change that could have unintended consequences.

On 9/05/2016, at 11:06 am, Walter notifications@github.com wrote:

To solve the problem, I add auto naming code to assign the class name to id, if beastObject.getID() == null or "".

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/CompEvol/beast2/issues/552#issuecomment-217751715

walterxie commented 8 years ago

Dear Alexei,

Yes. I has discussed with Remco. And I am going to send the email in a minute. Please be patient.

Cheers, Walter

On 9/05/16 11:08 am, "Alexei Drummond" notifications@github.com wrote:

Walter did you discuss this with any of the core team first? It seems like a change that could have unintended consequences.

On 9/05/2016, at 11:06 am, Walter notifications@github.com wrote:

To solve the problem, I add auto naming code to assign the class name to id, if beastObject.getID() == null or "".

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/CompEvol/beast2/issues/552#issuecomment-217751715

— You are receiving this because you modified the open/close state. Reply to this email directly or view it on GitHub https://github.com/CompEvol/beast2/issues/552#issuecomment-217751767

rbouckaert commented 8 years ago

Hi Walter,

If I understand the problem correct, the BEASTObject.toString() shows null if there is no ID, which is confusing in IDEs where it looks like the object is null, while only the ID is null.

On second thought, the way to solve this is by changing BEASTObject.toString() to test that getID() is null (and if so, return the class name, perhaps in lowercase), not XMLProducer. The XMLProducer fix ensures each object has an ID when it is converted to XML, but that does not help for IDEs.

walterxie commented 8 years ago

You are right, but if move the code into BEASTObject.toString(), it will be difficult to call XMLProducer.uniqueID(id, buf), which ensure ID is unique. And if so, you have to make it public static to be easy to call.

Cheers,

alexeid commented 8 years ago

I don’t think he means for you to move your code to BEASTObject.toString(). I think he means for you to also fix BEASTObject.toString() so that is still succeeds even if getID() returns null.

On 9/05/2016, at 11:48 am, Walter notifications@github.com wrote:

You are right, but if move the code into BEASTObject.toString(), it will be difficult to call XMLProducer.uniqueID(id, buf), which ensure ID is unique. And if so, you have to make it public static to be easy to call.

Cheers,

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/CompEvol/beast2/issues/552#issuecomment-217753476

rbouckaert commented 8 years ago

If you solving the "getID() return null" problem in an IDE, changing the XMLProducer does not solve this, but fixing BEASTObject.toString() does. I do not understand why calling XMLProducer.uniqueID(id, buf) needs to be called by toString() -- it can just return the class name.

walterxie commented 8 years ago

I confused the id validation with this toString problem. I will add the code to BEASTObject.toString().

On 9/05/16 12:04 pm, "Remco Bouckaert" notifications@github.com wrote:

If you solving the "getID() return null" problem in an IDE, changing the XMLProducer does not solve this, but fixing BEASTObject.toString() does. I do not understand why calling XMLProducer.uniqueID(id, buf) needs to be called by toString() -- it can just return the class name. — You are receiving this because you modified the open/close state. Reply to this email directly or view it on GitHub https://github.com/CompEvol/beast2/issues/552#issuecomment-217754247