Default and manual file names for header files included in generated code

[kerimoyle]

See discussion in #487 and #509.

Here's a test to show behaviour I'd expect from the generator in situations where an interface file is needed. Since we'll only generate code for a validated model, and since the validator checks the model name for emptiness/weird characters/etc, I think it's reasonable to go with the verbatim model name as the default rather than trying to change it to camel case or anything fancy. K.I.S.S :)

The test depends on implementation (are filenames stored/passed to the Generator or the GeneratorProfile?), so I wasn't sure how best to write it, but you get the general idea ...

TEST(Generator, setInterfaceFilenameAndDefault)
{
    auto model = libcellml::Model::create("myModelName");
    auto generator = libcellml::Generator::create();
    auto profileC = libcellml::GeneratorProfile::create(libcellml::GeneratorProfile::Profile::C);

    generator->setProfile(profileC);
    generator->processModel(model);

    std::string interfaceCode = generator->interfaceCode();
    EXPECT_TRUE(interfaceCode.find("#include \"myModelName.h\"") != std::string::npos)

    profileC->setInterfaceFileName("customHeaderFile.h"); // ?? or perhaps generator->setInterfaceFileName()??

    EXPECT_TRUE(interfaceCode.find("#include \"customHeaderFile.h\"") != std::string::npos)
    EXPECT_FALSE(interfaceCode.find("#include \"myModelName.h\"") != std::string::npos)
}

[agarny]

Here's a test to show behaviour I'd expect from the generator in situations where an interface file is needed. Since we'll only generate code for a validated model, and since the validator checks the model name for emptiness/weird characters/etc, I think it's reasonable to go with the verbatim model name as the default rather than trying to change it to camel case or anything fancy. K.I.S.S :)

Fine with the general idea (which indeed has to be done through GeneratorProfile), but not with using the name of a model as the default header file name. To have something like myModelName.h just looks wrong to me. For me, it should be mymodelname.h. So, because we are going to have the GeneratorProfile::setInterfaceFileNameString() method (and GeneratorProfile::interfaceFileNameString() one), I don't think we should have a default interface file name based on the name of the model.

[kerimoyle]

I don't understand why myModelName.h would be odd (especially if that's how the user chose to name their model?) And, as you say, it's able to be changed if other people find it weird too ... I guess overall I just think that anything is better than model.h. Does anyone else have thoughts/suggestions? @hsorby @nickerso

[agarny]

Odd because I am used to a different convention. Nothing more nothing less.

Also, thinking more about it, it means that here, we would need to do something that is specific to the C profile. Indeed, say that we were to do what you are after then mProfile->interfaceFileNameString() would, by default, return an empty string, meaning that we should use the name of the model. For one, what if there was to be no model name when calling the generator? Second, assuming that there is a model name, then we would need to append .h to it, which is specific to the C profile. So, what about if we were to have an Ada profile where the equivalent of a .h file is a .ads file (apparently since I don't know anything about Ada)? IOW, our generator wouldn't be language agnostic anymore, which is not good.

[nickerso]

In addition to @agarny's reasons, I think having a fixed default value, regardless of what it is, makes sense for the main case where people don't care - and fixed rather than based on model name so people are able to write code for working with the code strings without having to worry about different model names changing the code. If model.h isn't a good default then I'd suggest changing that rather than making it dynamic based on the model name. Perhaps something more self-describing like libcellml-generated-interface-code.h ?

Any user that cares about the file name should be using the setInterfaceFileNameString() method to set the name as the need.

[kerimoyle]

For one, what if there was to be no model name when calling the generator? @agarny There will always be a model name as valid CellML, because the validator checks before the generator proceeds. I thought about this before suggesting, and since CellML is pretty strict with the names we could be sure that there were no funky characters that would mess up a filename or path behaviour.

Second, assuming that there is a model name, then we would need to append .h to it, which is specific to the C profile. So, what about if we were to have an Ada profile where the equivalent of a .h file is a .ads file (apparently since I don't know anything about Ada)? IOW, our generator wouldn't be language agnostic anymore, which is not good.

I think I'm misunderstanding this one, sorry. Since you'd have a different header string for every language anyway, you could easily ask the user to specify the basename, and include the extension in the standard header text? eg:

headerTextForC = "#include \"<FILENAME>.h\""
headerTextForAda = "?include? \"<FILENAME>.ada\"" 

or, better, store the header extension as a variable specific to each profile? How do you currently change the syntax of the include statement, or do we assume that all languages which need an interface file use the #include syntax? I too have no idea about the syntax for Ada, but I looked it up quickly, and it's quite different:

INCLUDE CODE file1.ada
INCLUDE CODE ../file2.ada
INCLUDE PTU /usr/tests/file3.ptu

... in other words, this particular part of the code is not agnostic anyway, so there's no real difference?

Perhaps something more self-describing like libcellml-generated-interface-code.h ?

@nickerso I still worry about using static strings, especially because there's nothing anywhere in either implementation or interface code to say where it came from (see #501 ). Using a static filename means we're relying on people's naming of their files to figure out which interface and implementation code goes together ... and that feels dangerous to me. It also - and perhaps most importantly - means that we're still asking people to edit the generated file, which I'd thought we wanted to avoid.

A better solution IMHO would be for the generator to return an error if the filename had not been specified. This way everyone knows what's going on, and there are no surprises, no dodgy filenames, and no need to edit a generated file. What think you folks?

[agarny]

I think I'm misunderstanding this one, sorry. Since you'd have a different header string for every language anyway, you could easily ask the user to specify the basename, and include the extension in the standard header text?

Indeed, you could do that.

or, better, store the header extension as a variable specific to each profile?

Yes, this could also be done.

How do you currently change the syntax of the include statement, or do we assume that all languages which need an interface file use the #include syntax?

The code generator doesn't assume anything. The #include syntax can easily be changed by calling GeneratorProfile::setInterfaceHeaderString() and GeneratorProfile::setImplementationHeaderString().

A better solution IMHO would be for the generator to return an error if the filename had not been specified. This way everyone knows what's going on, and there are no surprises, no dodgy filenames, and no need to edit a generated file. What think you folks?

I think we are spending way too much time on something that is not worth it.

Right now, the Generator class allows to generate implementation code and, if needed, interface code. That code is returned to the user as a string. Then, it's up to the user to do whatever s/he wants with that string.

In case the user wants to generate C code, s/he can easily modify the implementation to specify the name of the file s/he wants to use for his/her interface file (through GeneratorProfile::setInterfaceFileNameString()).

So, unless I missed something, I believe we have everything in place to allow the user to generate code without the need for him/her to do any kind of postprocessing (this was already the case before PR #515 and still is after it). If that is not the case, then yes we should do something about it otherwise I don't think it's worth our time spending more time on this issue and on PR #515.