[Libraries] Remove unused functions from Utility and update cleanup comments #2913

johnsaigle · 2017-06-28T15:06:38Z

Delete functions marked as unused in the function comments (and confirmed by a Github search).
Update function comments to reflect some locations where they are used

driusan · 2017-06-28T15:12:58Z

This needs to go to 18.0. There's no way to determine that they're unused by any project overrides or instruments in the wild.

johnsaigle · 2017-07-04T18:07:34Z

Note: changes made by @AnyhowStep in #2889 have now been merged into this PR. The other PR will be closed.

johnsaigle · 2017-07-04T18:09:54Z

@ridz1208 This PR is an update of #2889 which was opened by @AnyhowStep. You said his PR should go to 17.2.

Do you think this should be in 17.2 or 18.0? My vote is 17.2 but @driusan said 18.0. Do you have a preference?

ridz1208 · 2017-11-23T15:37:30Z

php/libraries/Utility.class.inc

-     * @note    This is no longer used and should be removed
-     * @cleanup
-     */
-    static function getListOfSubjectEthnicities()


confirmed not being used

ridz1208 · 2017-11-23T15:40:03Z

php/libraries/Utility.class.inc

-     *       by NDB_Form_candidate_parameters and if so, should be moved there.
-     * @cleanup
-     */
-    static function explodeEncapsulated($seperator, $delimiter, $string)


used in candidate_parameters on IBIS

@sruthymathew123 if you can get rid of it or move it to the class it is used in, then when you confirm we'll be able to remove safely

ridz1208 · 2017-11-23T15:40:55Z

php/libraries/Utility.class.inc

-     * @note    Function comment written by Dave, not the author of this function.
-     * @note    This function does not appear to be used anymore.
-     * @cleanup
-     */


confirmed not used

ridz1208 · 2017-11-23T15:41:58Z

php/libraries/Utility.class.inc

-     * @return  The name from the given array row.
-     * @note    Function should be removed and code using it cleaned up.
-     * @cleanup
-     */


confirmed not used

ridz1208 · 2017-11-23T15:42:23Z

php/libraries/Utility.class.inc

-     * @return  The name from the given array row.
-     * @note    Function should be removed and code using it cleaned up.
-     * @cleanup
-     */


confirmed not used

ridz1208 · 2017-11-23T15:42:56Z

php/libraries/Utility.class.inc

-     *
-     * @cleanup
-     */
-    static function getColumnThresholdCount()


confirmed not used

ridz1208 · 2017-11-23T15:44:17Z

php/libraries/Utility.class.inc

-     *       module uses it. (Data team helper?)
-     * @note    Function comment written by Dave, not the author of this function.
-     * @cleanup
-     */


IBIS redefines this function in its ProjectUtility class but neither functions are used anywhere in the code

@sruthymathew123 potential clean-up for you !

ridz1208 · 2017-11-23T15:44:55Z

php/libraries/Utility.class.inc

-     *       module uses it. (Data team helper?)
-     * @note    Function comment written by Dave, not the author of this function.
-     * @cleanup
-     */


same...
IBIS redefines this function in its ProjectUtility class but neither functions are used anywhere in the code

@sruthymathew123 potential clean-up for you !

I think this was used by the really old "quat table" DQT. It's unused, and IBIS ProjectUtility probably just copied the Utility to start with.

ridz1208

In general the changes are fine, some of the comments are not constructive and should be removed

ridz1208 · 2017-11-23T15:48:04Z

php/libraries/Utility.class.inc

@@ -660,6 +574,10 @@ class Utility
     *                      to null.
     *
     * @return array The same array passed in, after modifications.


note should be removed

ridz1208 · 2017-11-23T15:48:17Z

php/libraries/Utility.class.inc

@@ -565,6 +544,8 @@ class Utility
     * @return  array If $var is an array, var, otherwise an array containing $var
     * @cleanup This should be removed and all uses converted to toArray
     *          (or vice versa, but toArray seems to be more common in the code)


note should be removed

ridz1208 · 2017-11-23T15:48:25Z

php/libraries/Utility.class.inc

@@ -546,6 +523,8 @@ class Utility
     * @param mixed $var The variable to be converted to an array.
     *
     * @return array If $var is an array, var, otherwise an array containing $var


note should be removed

ridz1208 · 2017-12-19T20:59:26Z

@zaliqarosli let's talk about this one

zaliqarosli · 2017-12-27T20:00:59Z

php/libraries/Utility.class.inc

    /**
     * Checks to see if a table contains a specified column
     *
     * @param string $table_name The table to check for a column.
     * @param string $column     The column name to check the table for.
     *
     * @return boolean true if the table has at least one NULL in the column
+     * @note   This should really be in the Database.class.inc file... -anyhowstep


I left this note in .. @ridz1208 should it be moved to Database?

probably, but not as part of this PR, and the note is still pointless to have. If anything, there should be a redmine ticket for it (or just a PR that does it and deprecates this)

I'm not against the notes, i tend to look at code more then redmine myself

zaliqarosli · 2017-12-27T20:02:15Z

This branch seems to be well out of sync, it doesn't generate a front-end for me. Need rebasing? @driusan @ridz1208 looks like its the Database class

driusan · 2018-01-03T14:39:30Z

php/libraries/Utility.class.inc

+     *       The return value format seems weird. Should possibly be refactored.
+     * @note    Used in:
+     *      - data_team_helper/php/NDB_Form_data_team_helper.class.inc
+     *      - data_team_helper/ajax/GetInstruments.php


A redmine ticket saying to move this to the data_team_helper and verify that that's the only place that it's used is better than a comment saying where it's used (which would need to be maintained, and may become outdated..)

ridz1208 · 2018-01-03T16:07:19Z

@zaliqarosli I will let you attempt a rebase on your local repo. if you encounter any problems let me know

cleaned up and added some notes gitignore Removed Utility::lookupBattery, There is already a NDB_BVL_Battery::lookupBattery and Utility version is not being used in core Restored Utility::lookupBattery(), tools/fix_timepoint_date_problems uses it Remove unused functions; update cleanup comments Phpcs and formatting update PHP version from 5 to 7 Remove note to self Remove invalid notes Remove notes

zaliqarosli · 2018-01-03T18:52:45Z

@ridz1208 please re-review. thanks!

ridz1208 · 2018-02-22T16:50:04Z

@driusan I think this should be merge early if you agree

johnsaigle added Release: Breaking changes PR that contains changes that might impact the code or accepted practices of active projects Cleanup PR or issue introducing/requiring at least one clean-up operation labels Jun 28, 2017

johnsaigle added this to the 17.2 milestone Jun 28, 2017

johnsaigle modified the milestones: 18.0, 17.2 Jun 28, 2017

johnsaigle changed the title ~~[Utility] Remove unused functions and update cleanup comments~~ [Libraries] Remove unused functions from Utility and update cleanup comments Jul 3, 2017

johnsaigle mentioned this pull request Jul 4, 2017

[Utility] Removed unused methods and added notes for future refactoring #2889

Closed

johnsaigle added the State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed label Jul 4, 2017

driusan removed this from the 19.0 milestone Oct 2, 2017

driusan added the [branch] major label Oct 2, 2017

ridz1208 modified the milestones: 19.1, 20.0 Oct 20, 2017

ridz1208 changed the base branch from 17.1-dev to major October 21, 2017 17:40

driusan self-requested a review November 22, 2017 21:44

ridz1208 reviewed Nov 23, 2017

View reviewed changes

php/libraries/Utility.class.inc

*

* @cleanup

*/

static function getColumnThresholdCount()

Copy link

Collaborator

ridz1208 Nov 23, 2017

Choose a reason for hiding this comment

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

confirmed not used

ridz1208 reviewed Nov 23, 2017

View reviewed changes

ridz1208 requested changes Nov 23, 2017

View reviewed changes

ridz1208 removed the State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed label Nov 23, 2017

ridz1208 mentioned this pull request Nov 23, 2017

[libraries/Utility] Add startsWith, endsWith string functions to Utility (with example) #2965

Closed

1 task

zaliqarosli self-assigned this Dec 26, 2017

zaliqarosli reviewed Dec 27, 2017

View reviewed changes

driusan reviewed Jan 3, 2018

View reviewed changes

ridz1208 approved these changes Feb 22, 2018

View reviewed changes

driusan merged commit 31dacf4 into aces:major Feb 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Libraries] Remove unused functions from Utility and update cleanup comments #2913

[Libraries] Remove unused functions from Utility and update cleanup comments #2913

johnsaigle commented Jun 28, 2017

driusan commented Jun 28, 2017

johnsaigle commented Jul 4, 2017

johnsaigle commented Jul 4, 2017 •

edited

Loading

ridz1208 Nov 23, 2017

ridz1208 Nov 23, 2017

ridz1208 Nov 23, 2017

ridz1208 Nov 23, 2017

ridz1208 Nov 23, 2017

ridz1208 Nov 23, 2017

ridz1208 Nov 23, 2017

ridz1208 Nov 23, 2017

driusan Jan 3, 2018

ridz1208 left a comment

ridz1208 Nov 23, 2017

ridz1208 Nov 23, 2017

ridz1208 Nov 23, 2017

ridz1208 commented Dec 19, 2017

zaliqarosli Dec 27, 2017

driusan Jan 3, 2018

ridz1208 Jan 3, 2018

zaliqarosli commented Dec 27, 2017 •

edited

Loading

driusan Jan 3, 2018

ridz1208 commented Jan 3, 2018

zaliqarosli commented Jan 3, 2018

ridz1208 commented Feb 22, 2018

[Libraries] Remove unused functions from Utility and update cleanup comments #2913

[Libraries] Remove unused functions from Utility and update cleanup comments #2913

Conversation

johnsaigle commented Jun 28, 2017

driusan commented Jun 28, 2017

johnsaigle commented Jul 4, 2017

johnsaigle commented Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ridz1208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ridz1208 commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaliqarosli commented Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

ridz1208 commented Jan 3, 2018

zaliqarosli commented Jan 3, 2018

ridz1208 commented Feb 22, 2018

johnsaigle commented Jul 4, 2017 •

edited

Loading

zaliqarosli commented Dec 27, 2017 •

edited

Loading