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

#389 virtual model properties for mocking accessibility #603

Closed
wants to merge 6 commits into from
Closed

#389 virtual model properties for mocking accessibility #603

wants to merge 6 commits into from

Conversation

Cpcrook
Copy link
Contributor

@Cpcrook Cpcrook commented Oct 11, 2019

Per issue #389 and PR #602 this is the second half of my fix for the referenced issue.

It makes all non-EventSource model class properties virtual so they can be mocked with a mocking framework such as Moq without issue. Combined with #602 this should allow effective mocking of a decent amount of this SDK.

In order to minimize impact to the code base, I decided against implementing interfaces for all of these model classes, as I don't believe there is any significant payoff to doing so as compared to virtual props.

@boxcla
Copy link

boxcla commented Oct 11, 2019

Verified that @Cpcrook has signed the CLA. Thanks for the pull request!

@carycheng
Copy link
Contributor

@Cpcrook, thank you for this pr! We really appreciate it, we will take a look at this and keep you updated

@PJSimon
Copy link
Contributor

PJSimon commented Oct 16, 2019

Hi @Cpcrook!
Thanks so much for this contribution! I just wanted to follow up and give you an expectation of time frame for our response.

The team is focused on a release this week for the box-ios-sdk. So, we will review your PR on Thursday of next week and will respond later that day!

Thanks again!

@CLAassistant
Copy link

CLAassistant commented Mar 3, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

mwwoda added 2 commits June 2, 2021 14:20
@mwwoda
merge with origin/main
6c7d622
@mwwoda
add missing methods to managers's interfaces
695a528 
add missing parameters to managers's interfaces
add obsolete attribute to managers's interfaces
add missing class descriptions to managers's interfaces
add virtual keyword to models that were missing it
@mwwoda
Copy link
Contributor

mwwoda commented Jun 2, 2021

Hi @Cpcrook!
Thank you once again for the contribution! I know that a lot time passed since the last update from us but we're working to integrate your changes into the main branch ASAP! I made some changes to your PR to keep it up-to-date with newest codebase, hope you don't mind. We plan to ship it with the next major release of the SDK!

@mwwoda

sujaygarlanka
sujaygarlanka previously approved these changes Jun 2, 2021
View reviewed changes
@mwwoda mwwoda mentioned this pull request Jun 8, 2021
Merged
@mwwoda
Copy link
Contributor

mwwoda commented Jun 8, 2021

I moved all the changes to separate PR because of appveyor issues when building from fork #735.

@mwwoda mwwoda closed this Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sujaygarlanka sujaygarlanka sujaygarlanka left review comments

@PJSimon PJSimon Awaiting requested review from PJSimon

@swfree swfree Awaiting requested review from swfree

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Cpcrook @boxcla @carycheng @PJSimon @CLAassistant @mwwoda @sujaygarlanka