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.

Sign up for GitHub

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

Jump to bottom

GT-129 Cast all returns from Config.php to appropriate types #420

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

GT-129 Cast all returns from Config.php to appropriate types #420

wants to merge 3 commits into from

Conversation

rowan04
Copy link
Member

@rowan04 rowan04 commented Jan 23, 2023

  • as per issue request, all returns from config.php are now explicitly cast to appropriate types
  • this is to prevent confusion and error as before returns were being cast as a SimpleXMLElement object which can cause unexpected behaviour
  • new lines were also added in and around some of the functions where they were missing to make the text more readable

Resolves #413

@rowan04
Cast all returns from Config.php to appropriate types
c8107a2 
- as per issue request, all returns from config.php are now explicitly cast to appropriate types
- this is to prevent confusion and error as before returns were being cast as a SimpleXMLElement object which can cause unexpected behaviour
- new lines were also added in and around some of the functions where they were missing to make the text more readable
@rowan04 rowan04 requested a review from a team as a code owner January 23, 2023 16:32
@tofu-rocketry
Copy link
Member

@gregcorbett I guess we don't have an existing unit test that could be extended to cover this?

@gregcorbett
Copy link
Member

I would presume not.

@gregcorbett
Copy link
Member

As discussed, @rowan04 would you be able to take a look at implementing a unit test to check the public methods in lib/Gocdb_Services/Config.php return the correct type and value for some given input?

@rowan04
Fix missing type cast on getHeadingTextColour function
89756eb 
- A type cast had accidentally not been added to the getHeadingTextColour function, so has now been added
@rowan04 rowan04 force-pushed the 413-cast-config-returns-to-appropriate-types branch from 54c3ace to 89756eb Compare January 26, 2023 16:27
@rowan04
Copy link
Member Author

rowan04 commented Jan 26, 2023

The force push was to amend a commit name/message

@rowan04
Copy link
Member Author

rowan04 commented Jan 26, 2023

After doing some tests on some of the functions in Config.php, this type casting method works. However, there is still some work for me to do to create some proper unit tests

gregcorbett
gregcorbett requested changes Feb 6, 2023
View reviewed changes
Copy link
Member

@gregcorbett gregcorbett left a comment

Choose a reason for hiding this comment

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

Unit test writing that is in progress.

@tofu-rocketry tofu-rocketry changed the title Cast all returns from Config.php to appropriate types GT-129 Cast all returns from Config.php to appropriate types Mar 7, 2023
@rowan04 rowan04 mentioned this pull request Sep 28, 2023
Open
@rowan04
Copy link
Member Author

rowan04 commented Sep 28, 2023

Unfortunately, due to me starting a new rotation, I am unable to write the unit tests for this pr. See #486

@rowan04
Copy link
Member Author

rowan04 commented Sep 29, 2023

fixed conflicts

@gregcorbett gregcorbett self-assigned this Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregcorbett gregcorbett gregcorbett requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@gregcorbett gregcorbett

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SimpleXML objects being returned from Config.php, not strings
3 participants
@rowan04 @tofu-rocketry @gregcorbett