Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instruments variable incorrectly defined #6140

Merged
merged 5 commits into from
Mar 10, 2020

Conversation

ridz1208
Copy link
Collaborator

@ridz1208 ridz1208 commented Mar 4, 2020

Brief summary of changes

This was showing up as an error on my IDE, the variable is defined as private in the hchild class while the parent class refers to it.

change from _select... to select... because static checks enforce the ugly _ before private variable rule

@ridz1208 ridz1208 added Category: Bug PR or issue that aims to report or fix a bug Cleanup PR or issue introducing/requiring at least one clean-up operation labels Mar 4, 2020
Copy link
Contributor

@lingma lingma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering whether it is necessary to change _selectMultipleElements to selectMultipleElements since the impacts to the existing instruments will be real. moreover, we have several other variables with the preifx _, obviously this is not mandatory to change right now.

If you change the name to without the prefix _, then there should be several more files to modify, not only these 2 files.

@@ -182,6 +182,11 @@ abstract class NDB_BVL_Instrument extends NDB_Page
*/
public $preview = false;

/**
* Array used by LINST instruments
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make the comment more descriptive? Used for what?

@ridz1208
Copy link
Collaborator Author

ridz1208 commented Mar 6, 2020

@lingma as per the description yes it is mandatory by our static analyses tools

@driusan I can but do you know what it's used for ? Or do I dig deeper ?

@lingma
Copy link
Contributor

lingma commented Mar 6, 2020

@lingma as per the description yes it is mandatory by our static analyses tools

Then you need to modify several others files including at least 3 instruments in raisinbread, some wifi pages, instruments templates, and add this as a warning in the release guide.

@ridz1208
Copy link
Collaborator Author

ridz1208 commented Mar 6, 2020

@lingma please request changes and specify which classes need to be modified. Thanks

@driusan
Copy link
Collaborator

driusan commented Mar 6, 2020

It looks like it's a list of multi-select elements kept so that they can be converted to/from {@} delimited on save.

Copy link
Contributor

@lingma lingma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs/deprecated_wifi/How-to-Code-an-instrument.md: line 14 and 31 (maybe not necessary if this is deprecated)
instruments/NDB_BVL_Instrument_TEMPLATE.class.inc
instruments/NDB_BVL_Instrument_UPLOADER_TEMPLATE.class.inc
raisinbread/instruments/* (3 of them) aosi.class.inc, medical_history.class.inc, mri_parameter_form.class.inc

and add name change to the release warning or release important change zone

@johnsaigle
Copy link
Contributor

I agee with Ling that the other files should be updated too. At least the Raisinbread instruments.

@johnsaigle johnsaigle added RaisinBread PR or issue introducing/requiring improvements to the Raidinbread dataset State: Needs RB update PR that needs to update Raisinbread to reflect its changes labels Mar 9, 2020
@ridz1208
Copy link
Collaborator Author

ridz1208 commented Mar 9, 2020

@lingma @johnsaigle does that cover everything ?

@johnsaigle johnsaigle removed the State: Needs RB update PR that needs to update Raisinbread to reflect its changes label Mar 9, 2020
@driusan
Copy link
Collaborator

driusan commented Mar 9, 2020

@ridz1208 the comment update is still missing

@lingma
Copy link
Contributor

lingma commented Mar 9, 2020

@ridz1208 the comment update is still missing

Right, the // is missing. In fact $this->selectMultipleElements = array(); or
//$this->selectMultipleElements = array(); should be the same.

@driusan
Copy link
Collaborator

driusan commented Mar 9, 2020

No, I mean the update to the comment that I requested be updated 3 days ago because it's not very descriptive still isn't very descriptive.

@ridz1208
Copy link
Collaborator Author

ridz1208 commented Mar 9, 2020

@lingma i removed that comment on purpose, it's better to just declare the empty array

@driusan done. I didnt specify how it gets manipulated cause that should be the function's description, not the variable. but I just added that it should contain all multiselect fieds

@driusan
Copy link
Collaborator

driusan commented Mar 9, 2020

@ridz1208 Makes sense to me, look better now.

php/libraries/NDB_BVL_Instrument.class.inc Outdated Show resolved Hide resolved
@driusan driusan merged commit a71b327 into aces:23.0-release Mar 10, 2020
@ridz1208 ridz1208 added this to the 23.0.0 milestone Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Bug PR or issue that aims to report or fix a bug Cleanup PR or issue introducing/requiring at least one clean-up operation RaisinBread PR or issue introducing/requiring improvements to the Raidinbread dataset
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants