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

Ci/aggressive linter #863

Merged
merged 40 commits into from
Aug 1, 2023
Merged

Ci/aggressive linter #863

merged 40 commits into from
Aug 1, 2023

Conversation

bdmendes
Copy link
Member

Closes #820
This sets up the very good analysis linter ruleset for the project, which enforces Dart's strict type-checking rules and more opinionated Flutter linting and formatting.
Although requiring a steeper adaptation to the project and the language, this will improve our consistency and allow for easier reviews and higher code quality overall.

Review checklist

  • Terms and conditions reflect the current change
  • Contains enough appropriate tests
  • If aimed at production, writes a new summary in whatsnew/whatsnew-pt-PT
  • Properly adds an entry in changelog.md with the change
  • If PR includes UI updates/additions, its description has screenshots
  • Behavior is as expected
  • Clean, well-structured code

@LuisDuarte1 LuisDuarte1 mentioned this pull request Jul 23, 2023
7 tasks
@LuisDuarte1 LuisDuarte1 marked this pull request as ready for review July 28, 2023 11:47
@LuisDuarte1
Copy link
Member

On this pr, I think it's appropriate to focus on making the linter pass without altering the app logic/architecture a lot. So, some rules were ignored that must be addressed in the future, and some calls to unawaited() (the linter requires that if you don't want to wait for the async function on a async context you must call this functions) that I don't know that are appropriate or not.

@bdmendes
Copy link
Member Author

On this pr, I think it's appropriate to focus on making the linter pass without altering the app logic/architecture a lot. So, some rules were ignored that must be addressed in the future, and some calls to unawaited() (the linter requires that if you don't want to wait for the async function on a async context you must call this functions) that I don't know that are appropriate or not.

Absolutely

@bdmendes
Copy link
Member Author

I have found that some things are still not ready, such as the test timing out and the course units page details not working. I'll draft this while these are not tackled, let me know if you find something @LuisDuarte1.

@bdmendes bdmendes marked this pull request as draft July 28, 2023 21:50
@bdmendes
Copy link
Member Author

@LuisDuarte1 I can't reproduce the course units detail page being broken on develop, however, I made the parser stricter here and fixed a bug you had on the query parameters parser. Please let me know if it is fixed for you now

Copy link
Contributor

@Sirze01 Sirze01 left a comment

Choose a reason for hiding this comment

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

Haven't read thoroughly trough the code because of the large amount of changes. However, I tried to test every feature and submitted my feedback and correction suggestions to @bdmendes, who has already integrated them.

Good job all.

Copy link
Contributor

@Process-ing Process-ing left a comment

Choose a reason for hiding this comment

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

From what I have tested, the app looks to be working as it should and the code looks good. Great job!

P.S.: Rip double quote :')

@bdmendes bdmendes merged commit 9bb7333 into develop Aug 1, 2023
4 checks passed
@bdmendes bdmendes deleted the ci/aggressive-linter branch August 1, 2023 18:52
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.

Make linter more aggressive (unused, formatting...)
4 participants