JeffersonLab / JANA2

Multi-threaded HENP Event Reconstruction
https://jeffersonlab.github.io/JANA2/
Other
6 stars 9 forks source link

Consistent naming #349

Closed DraTeots closed 4 weeks ago

DraTeots commented 3 months ago

Proposal: make JANA2 API naming consistent.

I believe, as of now, the most inconsistent part is member functions naming. All major API classes have PascalCase members, but some groups of classes have every function in snake_case (e.g. Topology or Engine folders).

Whatever JANA2 is going to end with: PascalCase or snake_case doesn't matter compared to how it looks now:


    auto app = new JApplication(new JParameterManager()); 
    app->AddPlugin("my_plugin");
    app->ProvideService(s);

   // And then...
    auto processor_arrow = new JEventProcessorArrow("processors", event_queue, nullptr, topology->event_pool);
    processor_arrow->add_processor(processor);
    block_source_arrow->attach(block_disentangler_arrow);
    block_disentangler_arrow->attach(processor_arrow);
    for (auto proc : topology->component_manager->get_evt_procs()) {
        processor_arrow->add_processor(proc);
   }

   // But back again
   app->Initialize();
   app->GetComponentSummary()
   app->GetService<RootFile_service>()

   // And this is the same framework...
nathanwbrei commented 3 months ago

Not inconsistent actually! The PascalCase methods are public-facing, and the snake_case methods are not. The example you pasted uses non-public-facing methods because the public ones didn't exist at the time.

DraTeots commented 2 months ago

So how to rewrite a piece above with public API and consistent naming?

This particular code:

https://github.com/JeffersonLab/JANA4ML4FPGA/blob/main/src/executables/jana4ml4fpga/CDAQTopology.h

DraTeots commented 4 weeks ago

What about this one?

void acquire_services(JServiceLocator *) override;

Is it public-facing enough to be in every user written service?

DraTeots commented 4 weeks ago

Ok =) Found this in the source:

    // This will be deprecated eventually
    virtual void acquire_services(JServiceLocator*) {};

Thanks!

nathanwbrei commented 4 weeks ago

acquire_services() will be replaced by Init(), once I update eicrecon and hd_recon so as not to produce tons of deprecation warnings. However, Init() itself is less necessary because parameters and services should be injected using the Parameter<>() and Service<> helpers, just like JOmniFactory. As far as I'm aware, all components now support injecting parameters and services this way. This is part of #276.