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

Migrate group_domain_models package null safety #495

Merged
merged 18 commits into from
Aug 17, 2023
Merged

Migrate group_domain_models package null safety #495

merged 18 commits into from
Aug 17, 2023

Conversation

nilsreichardt
Copy link
Member

Closes #192

@github-actions github-actions bot added dependencies Changing, updating, adding or removing one or more dependencies. testing labels Mar 12, 2023
@github-actions
Copy link

github-actions bot commented Mar 12, 2023 •
edited
Loading

Visit the preview URL for this PR (updated for commit b20b368):

https://sharezone-test--pr495-migrate-group-domain-xb47pm61.web.app

(expires Thu, 17 Aug 2023 00:26:13 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 4cb3ae61e1e018abfd9841fd3239f5b49ccc034b

Jonas-Sander
Jonas-Sander requested changes Mar 12, 2023
View reviewed changes
lib/group_domain_models/lib/src/models/course.dart Outdated Show resolved Hide resolved
lib/group_domain_models/lib/src/models/course.dart Outdated Show resolved Hide resolved
lib/group_domain_models/lib/src/models/course.dart Outdated Show resolved Hide resolved
lib/group_domain_models/lib/src/models/school.dart Outdated Show resolved Hide resolved
lib/group_domain_models/lib/src/models/school.dart Outdated Show resolved Hide resolved
lib/group_domain_models/lib/src/models/school_class.dart Outdated Show resolved Hide resolved
lib/group_domain_models/lib/src/models/school_class.dart Outdated Show resolved Hide resolved
lib/group_domain_models/lib/src/models/school_class.dart Outdated
Comment on lines 143 to 144
required String id,
required Map<String, dynamic> data,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit (maybe for some other PR): For some classes data is positional and id is named and for example here both is named. We should decide on one of both for all cases.

lib/group_domain_models/lib/src/models/write_permissions.dart Outdated
Comment on lines 17 to 19
default:
return 'Einstellung konnte nicht gefunden werden';
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't we only use both cases in the switch and put the fallback as a separate return after it so that if a new WritePermission case is introduced we get a warning here?

@github-actions github-actions bot added feature: timetable / calendar Includes anything regarding lessons (timetable) and events (calendar). feature: groups:courses Specific to only courses (instead of e.g. classes) feature: groups:classes Specific to only classes (instead of e.g. courses) feature: groups Groups umbrella term for courses and classes. feature: group permissions Group admins can set what permissions different course members can do (e.g. only read content). labels Aug 8, 2023
Jonas-Sander
Jonas-Sander approved these changes Aug 8, 2023
View reviewed changes
Copy link
Collaborator

@Jonas-Sander Jonas-Sander left a comment

Choose a reason for hiding this comment

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

@nilsreichardt Do you want to look at it again or should I go ahead and merge?

nilsreichardt
nilsreichardt commented Aug 8, 2023
View reviewed changes
Copy link
Member Author

@nilsreichardt nilsreichardt left a comment

Choose a reason for hiding this comment

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

LGTM

@Jonas-Sander Jonas-Sander enabled auto-merge August 8, 2023 18:24
@Jonas-Sander Jonas-Sander added this pull request to the merge queue Aug 8, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Aug 8, 2023
@nilsreichardt nilsreichardt added this pull request to the merge queue Aug 8, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 8, 2023
@Jonas-Sander Jonas-Sander added this pull request to the merge queue Aug 9, 2023
@Jonas-Sander Jonas-Sander removed this pull request from the merge queue due to a manual request Aug 9, 2023
@Jonas-Sander Jonas-Sander enabled auto-merge August 9, 2023 16:57
@Jonas-Sander Jonas-Sander added this pull request to the merge queue Aug 9, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 9, 2023
@nilsreichardt nilsreichardt enabled auto-merge August 9, 2023 20:05
@nilsreichardt nilsreichardt added this pull request to the merge queue Aug 9, 2023
@nilsreichardt nilsreichardt removed this pull request from the merge queue due to a manual request Aug 9, 2023
@nilsreichardt nilsreichardt added this pull request to the merge queue Aug 10, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 10, 2023
@nilsreichardt nilsreichardt added this pull request to the merge queue Aug 17, 2023
Merged via the queue into main with commit 54619c5 Aug 17, 2023
@nilsreichardt nilsreichardt deleted the migrate-group-domain-models branch August 17, 2023 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jonas-Sander Jonas-Sander Jonas-Sander approved these changes

Assignees
No one assigned
Labels
dependencies Changing, updating, adding or removing one or more dependencies. feature: group permissions Group admins can set what permissions different course members can do (e.g. only read content). feature: groups:classes Specific to only classes (instead of e.g. courses) feature: groups:courses Specific to only courses (instead of e.g. classes) feature: groups Groups umbrella term for courses and classes. feature: timetable / calendar Includes anything regarding lessons (timetable) and events (calendar). testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate the group_domain_models package to null safety
2 participants
@nilsreichardt @Jonas-Sander