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.

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

Reorganize Core files #3040

Merged
merged 14 commits into from
Jan 9, 2025
Merged

Reorganize Core files #3040

merged 14 commits into from
Jan 9, 2025

Conversation

rh12
Copy link
Contributor

@rh12 rh12 commented Dec 20, 2024

affects: Student, Teacher, Parent
release note: none

Reorganized the files in Core project. See screenshot below.

  • Everything is under Common and Features now.
  • Fixed some filename errors, in rare cases updated the classname itself.
  • Split some files, mostly extensions.
  • Didn't refactor anything, didn't touch any logic.
  • Synced CoreTests structure with Core structure.
  • Cleaned up coverage/config.json

Some notes:

  • Didn't try to straighten out Logger vs Analytics vs ScreenViewLogger vs PageViewAnalytics. But they are all in CommonModels and CommonUI now.
  • Didn't try to straighten out SwiftUIViews, ViewModifiers, UIViews, ViewModel, Viewables, etc. Plus there is InstUI. But they are all in CommonUI now.
  • CommonUI has both UIKit and SwiftUI related types. It was not trivial to separate the two, so I left them together.
  • Didn't try to reorganize or straighten out the features themselves.

Test plan

Build and run all three apps.
I suggest to checkout and see locally because on github it's slow.

Screenshots

@inst-danger
Copy link
Contributor

inst-danger commented Dec 20, 2024

Teacher Build QR Code:

@inst-danger
Copy link
Contributor

inst-danger commented Dec 20, 2024

Parent Build QR Code:

@inst-danger
Copy link
Contributor

inst-danger commented Dec 20, 2024

Student Build QR Code:

@inst-danger
Copy link
Contributor

inst-danger commented Dec 20, 2024

Fails
🚫

Please add a reference to a Jira ticket. For example: refs: MBL-10023

Warnings
⚠️ This pull request will not generate a release note.
⚠️ One or more files are below the minimum test coverage 50%

Affected Apps: Student, Teacher, Parent

Coverage New % Master % Delta
Canvas iOS 91.28% 91.24% 0.04%
Core/Core/Common/CommonModels/Store/FetchedCollection.swift 48.72% -- --
Core/Core/Common/Extensions/Foundation/CGSizeExtensions.swift 0% -- --
Core/Core/Features/Courses/SmartSearch/Model/CourseSmartSearchViewAttributes.swift 30.77% -- --
Core/Core/Common/Extensions/SwiftUI/UIColor+Color.swift 0% -- --
Core/Core/Features/Search/Model/SearchSupportButtonModel.swift 0% -- --
Core/Core/Common/CommonModels/AppEnvironment/AppEnvironmentOverride.swift 32.76% -- --

Generated by 🚫 dangerJS against 76e9de6

Copy link
Contributor

@suhaibabsi-inst suhaibabsi-inst left a comment

Choose a reason for hiding this comment

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

Checking out this branch on my machine, I had the following issue:
1- An empty "Planner/CalendarEvent" folder found beneath "Features" & "Common"
2- Two "Assets" files was found, at "Core" and "Core/Resources". I am wondering whether we need such arrangement?

Screenshot 2024-12-23 at 12 46 39 PM

@rh12
Copy link
Contributor Author

rh12 commented Jan 6, 2025

Hi @suhaibabsi-inst

Checking out this branch on my machine, I had the following issue:
1- An empty "Planner/CalendarEvent" folder found beneath "Features" & "Common"

This is probably caused by an empty folder in your local repository. Git ignores empty folders but the project generation script doesn't. Please just remove the folder itself. It should not be there on a fresh checkout/clone.

2- Two "Assets" files was found, at "Core" and "Core/Resources". I am wondering whether we need such arrangement?
I believe it's the same. There is probably a leftover empty Assets.xcassets folder.

I don't have any of these and since we can't affect empty folders in a commit I believe a local cleanup may be needed (or a clean checkout) if you have some leftovers.

@rh12 rh12 force-pushed the hackweek/Reorganize-Files branch from 3630268 to 7209dc8 Compare January 6, 2025 10:48
Copy link
Contributor

@vargaat vargaat left a comment

Choose a reason for hiding this comment

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

I welcome these changes, thanks for taking the time to do them! Feedback:

  1. I think we could omit the Common prefix on subfolders. I would also stick to the MVVM layer naming and rename Models to Model and the UI folder to View as we have with other modules.
    Screenshot 2025-01-06 at 16 25 01

  2. We could also omit the Extensions suffix from these directories. I don't feel the added value but it's harder to read.
    Screenshot 2025-01-06 at 16 32 17

@rh12
Copy link
Contributor Author

rh12 commented Jan 7, 2025

  1. I think we could omit the Common prefix on subfolders. I would also stick to the MVVM layer naming and rename Models to Model and the UI folder to View as we have with other modules.

The Common prefix felt a bit more expressive to me in this case, but I'm not entirely against removing it.
I thought about Model/View originally, but it doesn't really follow MVVM in this level. Event though Models contain only Model level code, the UI contains all layers within each component. So I would intentionally keep it UI (or CommonUI).
Separation of components and organization was not clearcut here, thing could be improved more later on I think.

  1. We could also omit the Extensions suffix from these directories. I don't feel the added value but it's harder to read.

I agree, will change them! (note to self: Webkit -> WebKit)

@szabinst
Copy link
Contributor

szabinst commented Jan 7, 2025

Since we have Resources folder, I'd introduce Sources folder for everything else.

I'd also drop the prefixes but I'm hesitant on the MVVM splitting here because they are not really implemented that way.

In Horizon, I did split the common folder but it was easier because everything was written from scratch with separation of concerns in mind.

Screenshot 2025-01-07 at 12 43 44

szabinst
szabinst previously approved these changes Jan 8, 2025
@rh12 rh12 force-pushed the hackweek/Reorganize-Files branch from c8910f8 to 76e9de6 Compare January 9, 2025 09:43
@rh12
Copy link
Contributor Author

rh12 commented Jan 9, 2025

As discussed on Slack with @vargaat and @szabinst we will leave Common folder naming as it is and consider further organization/cleanup for its contents later on.

Copy link
Contributor

@ndrsszsz ndrsszsz left a comment

Choose a reason for hiding this comment

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

Nice! 👍

@rh12 rh12 merged commit badd118 into master Jan 9, 2025
5 of 6 checks passed
@rh12 rh12 deleted the hackweek/Reorganize-Files branch January 9, 2025 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants