Classes do not follow Ruby convention.

[natacha-beck]

Some classes do not follow Ruby naming convention.

It is the case for CSVFile, YAMLFile and XMLfile, it will be nice to rename it properly, BUT this will require to have a migration in CBRAIN in order to change the class of all existing CSV, XML and YAML file.

[prioux]

Yes, it's a good issue, Natacha and I identified it while reviewing her viewer PR last week.

The code in all the plugins must be changed too so that the class names are properly changed.

[prioux]

1. Files to fix:

./BrainPortal/app/controllers/nh_loris_hooks_controller.rb
./BrainPortal/app/helpers/scope_helper.rb
./BrainPortal/cbrain_plugins/cbrain-plugins-base/userfiles/cbrain_file_list/cbrain_file_list.rb
./BrainPortal/cbrain_plugins/cbrain-plugins-base/userfiles/csv_file/csv_file.rb
./BrainPortal/cbrain_plugins/cbrain-plugins-base/userfiles/tsv_file/tsv_file.rb
./BrainPortal/cbrain_plugins/cbrain-plugins-neuro/cbrain_task/fsl_melodic/portal/fsl_melodic.rb
./BrainPortal/cbrain_plugins/cbrain-plugins-base/userfiles/yaml_file/yaml_file.rb
./BrainPortal/cbrain_plugins/cbrain-plugins-base/userfiles/xml_file/xml_file.rb

Note that one of these files is in a plugin

2. Database migration

• make sure it's reversible
• update the type attribute directly, don't use the full ActiveRecord lifecycle methods

[MontrealSergiy]

what about CBRAIN class? Along with CBRAINExtensions?

FSLDesignFile?

Honestly nobody objecting recent addition of MADEOutput and OSPREYOutput make me think the issue is of low value

[prioux]

Yes, it's true we have a bunch of older code that are not following the conventions. That is because when we were learning Ruby, Tarek and I didn't think about these things. But that is no excuse for not doing it properly when adding new code nowadays.

We will eventually fix the older classes (CBRAIN, CBRAINExtensions etc).

[MontrealSergiy]

By update the type attribute directly you mean use SQL rather than update_column or save! ?

[prioux]

No I mean use update_column(), it's fine. save() wouldn't work. Remember that in the migration you'll have the DB and the model classes in two states, incompatibles, so you have to change them without loading the records.

[MontrealSergiy]

ok. Scary, but can try.

[prioux]

Write your migration code so that it works just on one class, e.g. 'XMLFile' first, that way if anything goes wrong it's easy to undo manually (especially if you make sure you only have one such file in your DB)

[MontrealSergiy]

@natacha-beck Hi, I am curios why do you think that XMLFile follows Ruby conventions better than XmlFile. From what I see most ruby style guides recommend keep abbreviation as is, e.g XMLFile (even though all them erroneously call abbreviations acronyms)

https://rubystyle.guide/

Keep acronyms like HTTP, RFC, XML uppercase

I even see a core IO class

[prioux]

@MontrealSergiy It is a very good point. I am now reconsidering renaming things.

We should apply this change only to things that are clearly against style. So yes, we keep XMLFile and CSVFile... !!!

[natacha-beck]

Yes, please keep XML please.

But should we rane the JsonFile to JSONfile then ?