Skip to content

Refactor/adding data sources abstractions #87

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
Merged
TorinAsakura merged 14 commits into from
Jan 11, 2025
Merged

Refactor/adding data sources abstractions #87

TorinAsakura merged 14 commits into from
Jan 11, 2025

Conversation

@looee1q
Copy link
Contributor

@looee1q looee1q commented Dec 18, 2024

@looee1q looee1q self-assigned this Dec 18, 2024
@looee1q looee1q force-pushed the refactor/adding-data-sources-abstactions branch from c0a92e5 to 007b80d Compare December 20, 2024 07:49
@looee1q
Copy link
Contributor Author

looee1q commented Dec 20, 2024 •
edited
Loading

@TorinAsakura В ходе работы над этой таской выявил следующие моменты, которые хотелось бы подстветить. Они же служат обоснованием почему я написал код именно так.

  1. Текущий NetworkDataSource по сути является оберткой для baseUrl. Полагаю, цель внедрения NetworkDataSource была все-таки другой. Вероятно в этом классе должна происходить настройка инструмента для работы с сетью. А NetworkRepositoryImpl должен опираясь на этот инструмент образовывать функции, не зависящие от того, каким конкретно будет инструмент для работы с сетью, и предоставлять их UseCase(ам) и дальше в SDK. Текущая реализация NetworkDataSource на мой взгляд не имеет никакого смысла. Я бы занялся отделением кода NetworkRepositroy от NetworkDataSource при внедрении Ktor. А на данном этапе просто удалил NetworkDataSource.
  2. Тут случайно продублировали jdoc. Удалил лишние комментарии.
  3. Почему внутри рабочих функций (не являющихся deprecated) используются deprecated фунции? Вот пример: внутри функции profile() используется deprecated функция sendAsync(). Такого ведь нельзя допускать?

Дополнительно еще вынес два момента в дисуссии для обсуждения.

@looee1q looee1q requested a review from TorinAsakura December 20, 2024 11:39
@TorinAsakura
Copy link
Contributor

TorinAsakura commented Dec 24, 2024

@TorinAsakura В ходе работы над этой таской выявил следующие моменты, которые хотелось бы подстветить. Они же служат обоснованием почему я написал код именно так.

  1. Текущий NetworkDataSource по сути является оберткой для baseUrl. Полагаю, цель внедрения NetworkDataSource была все-таки другой. Вероятно в этом классе должна происходить настройка инструмента для работы с сетью. А NetworkRepositoryImpl должен опираясь на этот инструмент образовывать функции, не зависящие от того, каким конкретно будет инструмент для работы с сетью, и предоставлять их UseCase(ам) и дальше в SDK. Текущая реализация NetworkDataSource на мой взгляд не имеет никакого смысла. Я бы занялся отделением кода NetworkRepositroy от NetworkDataSource при внедрении Ktor. А на данном этапе просто удалил NetworkDataSource.
  2. Тут случайно продублировали jdoc. Удалил лишние комментарии.
  3. Почему внутри рабочих функций (не являющихся deprecated) используются deprecated фунции? Вот пример: внутри функции profile() используется deprecated функция sendAsync(). Такого ведь нельзя допускать?

Дополнительно еще вынес два момента в дисуссии для обсуждения.

  1. Всё так. Поддерживаю.
  2. Конечно же нельзя. Сжечь их…Сжечь их всех!

TorinAsakura
TorinAsakura suggested changes Dec 24, 2024
View reviewed changes
@looee1q
refactor(JSONObject): Added functions to add queries params to JSONOb…
a405ba7 
…ject.
@looee1q
refactor(UserSettingDataSourceImpl): Removed isInitialized field and …
ebc2402 
…all corresponding methods.
@looee1q
refactor(UserSettingDataSource): Removed UserSettingsDataSource classes
ba83c2b 
Logic from UserSettingsDataSource moved directly into UserSettingsRepository.
All basic user parameters (shop_id, seance, segment, did, sid) goes through SharedPreferences.
@looee1q looee1q force-pushed the refactor/adding-data-sources-abstactions branch from 0d60144 to ba83c2b Compare January 9, 2025 12:15
@looee1q
fix(UserSettingsRepositoryImpl): Fix key for saving did with SharedPref
6a58863 
Logic from UserSettingsDataSource moved directly into UserSettingsRepository.
All basic user parameters (shop_id, seance, segment, did, sid) goes through SharedPreferences.
@looee1q
Merge branch 'master' into refactor/adding-data-sources-abstactions
7913be8 
# Conflicts:
#	personalization-sdk/src/main/kotlin/com/personalization/SDK.kt
#	personalization-sdk/src/main/kotlin/com/personalization/sdk/data/repositories/userSettings/UserSettingsDataSource.kt
#	personalization-sdk/src/main/kotlin/com/personalization/sdk/data/repositories/userSettings/UserSettingsRepositoryImpl.kt
#	personalization-sdk/src/main/kotlin/com/personalization/sdk/domain/repositories/UserSettingsRepository.kt
#	personalization-sdk/src/main/kotlin/com/personalization/sdk/domain/usecases/userSettings/GetUserSettingsValueUseCase.kt
@looee1q
refactor: Conflict resolving after merging master
7d909e0
@looee1q looee1q requested a review from TorinAsakura January 10, 2025 18:11
@looee1q looee1q requested a review from TorinAsakura January 10, 2025 19:53
@TorinAsakura TorinAsakura merged commit 78d5741 into master Jan 11, 2025
1 check passed
@TorinAsakura TorinAsakura deleted the refactor/adding-data-sources-abstactions branch January 11, 2025 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@TorinAsakura TorinAsakura TorinAsakura approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@looee1q looee1q

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@looee1q @TorinAsakura