Skip to content

Miscellaneous improvements #1

Open
Narzgul wants to merge 1 commit intoIsAvaible:mainfrom
Narzgul:main
Open

Miscellaneous improvements #1
Narzgul wants to merge 1 commit intoIsAvaible:mainfrom
Narzgul:main

Conversation

@Narzgul
Copy link

@Narzgul Narzgul commented Apr 1, 2023

Let me start with excusing myself for this undocumented contribution and I am willing to continue explaining my changes if necessary.

This is the code I use to communicate to the Webuntis-API in my HUntis App. It is forked from this project and many of the changes are for adherence to the flutter code-style.

I however also have added some changes that could potentially clash with the usage of webuntis in other schools / institutions. Since I have only developed my app and by extension added to this project only with access to my schools system I can not check if it works for others.

I want to finish with expressing my gratitude to @IsAvaible for providing this project, because my app would not have been possible without it, which is why I want to give back by contributing!

@IsAvaible
Copy link
Owner

Hey @Narzgul,

Thank you for your interest in my project! I have looked into the changes you have made and have found some issues that would need to be resolved before I could consider merging your pull request:

  1. You introduced three unused imports ('package:flutter/material.dart', 'package:huntis/main.dart', 'package:intl/intl.dart'), one of which is part of your personal project ( 'package:huntis/main.dart').
  2. Most of your changes are of a stylistic / formatting nature. While I do agree, that some parts of the original code are too dense and hard to read, line splits like [untis.dart - line:574] feel unnecessary to me. Maybe we could agree on a code style / formatting tool to use?
  3. I have found one snippet that seems to impact the behavior of the code [untis.dart - line:150-243]. Some changes seem to be specific to your project (e.g. [untis.dart - line:200]). Could you elaborate on what exactly the purpose of the snippet is?

Have a nice Sunday!

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.

2 participants