onebusaway-ios icon indicating copy to clipboard operation
onebusaway-ios copied to clipboard

Published 20 hours ago • verified publisher icon OneBusAway

Fix: issue 629

Open aaryankotharii opened this issue 1 year ago • 8 comments

This PR migrates regions storage from UserDefaults to FileManager. Same file structure followed as mentioned in issue description.
fixes issue: #629

Code Changes

  • Created FileManagerProtocol to abstract FileManager operations for testability.
  • Updated RegionService to store and fetch defaultRegion and custom Regions in File system.
  • Created FileManagerMock and updated RegionsServiceTests.

aaryankotharii avatar Feb 23 '24 04:02 aaryankotharii

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Feb 23 '24 04:02 CLAassistant

Thank you @aaryankotharii! I'll review and get back to you soon!

aaronbrethorst avatar Feb 24 '24 00:02 aaronbrethorst

Hello @ualch9, I have made the following changes as requested above.

  1. replaced PropertyListEncoder / Decoder with JSONEncoder / Decoder.
  2. replaced enum FileDestination logic with simple static constants in RegionsService.
  3. renamed FileManagerProtocol to RegionsServiceFileManagerProtocol.

aaryankotharii avatar Mar 17 '24 00:03 aaryankotharii

Thank you @aaryankotharii! Apologies for the delayed turnaround in reviewing, I'll get to this ASAP.

ualch9 avatar Mar 17 '24 17:03 ualch9

no worries @ualch9, but there are a few migration issues I'm not sure how to address. for example, how to approach fetching custom regions, of existing users, which are stored in userdefaults ?

aaryankotharii avatar Mar 17 '24 17:03 aaryankotharii

no worries @ualch9, but there are a few migration issues I'm not sure how to address. for example, how to approach fetching custom regions, of existing users, which are stored in userdefaults ?

Excellent question... @aaronbrethorst, do you have any insight on if custom regions are used enough for us to support a migration path?

ualch9 avatar Mar 17 '24 18:03 ualch9

I would not worry about custom regions at this time. Thanks for checking!

aaronbrethorst avatar Mar 17 '24 18:03 aaronbrethorst

@aaryankotharii - need anything from me in order to get this wrapped up?

aaronbrethorst avatar Apr 11 '24 02:04 aaronbrethorst