Code cleanup

[lesquoyb]

I'm currently cleaning the code base helped by spotbugs and other code-analysis tools. Feel free to participate if you want. Also the scope of this rework is super wide so do not hesitate to review the parts of the code you have some knowledge over as I may (and surely will) introduce new bugs.

[lesquoyb]

@benoitgaudou I've noticed a class GamaSqlConnection in the database plugin that seems to never be used, do you have an idea what it is ? Is it something we can safely delete ?

[lesquoyb]

@ptaillandier I deleted the variables that were marked as deprecated in driving skill, but now I need to rewrite the example models so they follow the new architecture, could you have a look ? I don't think it's much to do, we just need to replace the use of current_lane in Traffic.gaml and Simple intersection.gaml

[lesquoyb]

@AlexisDrogoul As the facets parameter and category of variables where marked as soon to be deprecated I removed them from the code base and models in the library. Though there's a case that I didn't think about and now I don't know what to do with: in case of declaration of a variable inside an experiment it is not possible anymore to use those facets, so we have to do it in two times declaration of the variable then declaration of the parameter. But in that case it feels a bit weird as most experiment variables should be experiment parameters (at least from my experience).

What do you think ? Should we keep that two phases declaration even in the experiment, that could make sense as it's also not very common to declare experiment variables anyway, or should we reintroduce parameter and category but only in an experiment block ? In the latter case I might need your help because I don't really know how to properly constrain a facet to be used only in a specific block

[AlexisDrogoul]

Tough question. Actually, and even though they were marked as soon to be deprecated, these facets had an interest : that of forcing every experiment to have the attribute as parameter, instead of having to declare it in all of them.

[lesquoyb]

Yes, though there's still the possibility to create a base experiment and inherit it later. The advantage is that we can also ignore it if we want to restrain the parameters to set for the user

[lesquoyb]

@AlexisDrogoul I split the Spatial class into multiple classes that I moved into a new package gama.gaml.operators.spatial. But since then I have some weird problems with the autogenerated GamlAdditions in all projects depending on them. The GamlAdditions files generated all contain old references to gama.gaml.operators.Spatial.Properties and gama.gaml.operators.Spatial.* which do not exist anymore. I recreated a workspace to get rid of this but it didn't work. I found that those strings probably originated from Cosntants.java and GamaProcessor.java in gama.processor so I replaced them too with the current version, but again no difference.

With @RoiArthurB we tried to make work Generate_xtext13.mwe2 in the hope that it would solve the issue but we weren't able to run it.

Do you have any idea what I can do to fix this ?

[AlexisDrogoul]

Is it really necessary to split the Spatial class into subclasses ? Le 4 avr. 2024 à 14:46, Baptiste Lesquoy @.**> a écrit : @AlexisDrogoul I split the Spatial class into multiple classes that I moved into a new package gama.gaml.operators.spatial. But since then I have some weird problems with the autogenerated GamlAdditions in all projects depending on them. The GamlAdditions files generated all contain old references to gama.gaml.operators.Spatial.Properties and gama.gaml.operators.Spatial. which do not exist anymore. I recreated a workspace to get rid of this but it didn't work. I found that those strings probably originated from Cosntants.java and GamaProcessor.java in gama.processor so I replaced them too with the current version, but again no difference. With @RoiArthurB we tried to make work Generate_xtext13.mwe2 in the hope that it would solve the issue but we weren't able to run it. Do you have any idea what I can do to fix this ?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

[lesquoyb]

Well it's an almost 8000 lines of code file that is already split in multiple classes representing semantic categories inside, the Spatial class itself doesn't contain any code so it's just a container. So while cleaning up the code base why not go a bit further and make it as many independent classes ? It's easier to understand and thus maintain.

[lesquoyb]

@AlexisDrogoul I noticed there are two classes BinarySerialisation in the serialize plugin, on in implementations and one in binary, apart from one line:

private static FSTBinaryProcessor PROCESSOR = new FSTBinaryProcessor();

or

private static BinarySerialiser PROCESSOR = new BinarySerialiser();

Both classes are used in different places in the code. Is there still a reason to keep those two implementations ? and if so couldn't we refactor and just have the processor as a parameter or something like this ?

[lesquoyb]

@ptaillandier I've implemented the changes we talked about in the path_between operator, but now some of the unit tests in relation.experiment are not working anymore. I'm not sure if the change we brought is faulty or if the tests were wrong to begin with, could you have a look please ?