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

feat(neon_framework): use logging framework instead of print #1642

Merged
merged 4 commits into from
Mar 2, 2024

Conversation

Leptopoda
Copy link
Member

implements: #608

@provokateurin
Copy link
Member

I guess we should have a script that checks that print and debugPrint are never used in any of the neon_* packages?

@Leptopoda Leptopoda force-pushed the feat/neon/logging_framework branch from d3f5b05 to fdf0847 Compare February 24, 2024 18:18
@Leptopoda
Copy link
Member Author

Not sure if you already had a look at the latest commit but I've tried to address the suggestion from your other #1642 (comment) I've opted to use the custom_lint package which provides an abstraction to write custom lint rules using the analyzer api (similar to what the analyzer itself uses).

I had to bump the project sdk version to 3.2.0 (not the for the packages though) and the analyzer plugin api is still experimental. You can read more about it here https://dart.dev/tools/analysis#plugins.
There are some limitations like recommended 16GB of RAM and and not more than 10 packages in a monorepo. I'd say let's see how far we can go with it and if desired we can still switch it to regexes.

A benefit of this approach is that we can write custom quickFixes for things like dart:io=> universal_io, higher accuracy as we can use the analyzer directly and direct feedback while developing (I never run these scripts before submit 😅)

@provokateurin
Copy link
Member

Yeah I really liked that you added this, it looks like a pretty cool feature to use. I don't like that we have to use a custom script to do the analyzing, but I guess that's a trade off worth it.

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Two things:

  1. In a lot of places where errors are caught only the info or warning levels are used, but they should be severe instead. I know that some places can stay info or warning, but most should really be severe.
  2. My IntelliJ doesn't pick up the custom linting rule, do I need to change something so it works? (Yes I restarted the analyzer and the whole IDE).

packages/neon_lints/lib/neon_lints.dart Outdated Show resolved Hide resolved
packages/neon_lints/lib/neon_lints.dart Outdated Show resolved Hide resolved
packages/neon_lints/lib/src/amend_model_suffix.dart Outdated Show resolved Hide resolved
packages/neon_lints/lib/src/amend_model_suffix.dart Outdated Show resolved Hide resolved
packages/neon_lints/lib/src/amend_model_suffix.dart Outdated Show resolved Hide resolved
packages/neon_lints/lib/src/amend_model_suffix.dart Outdated Show resolved Hide resolved
packages/neon_framework/lib/src/blocs/next_push.dart Outdated Show resolved Hide resolved
packages/neon/neon_files/lib/src/blocs/files.dart Outdated Show resolved Hide resolved
packages/neon_framework/lib/src/blocs/user_status.dart Outdated Show resolved Hide resolved
packages/neon_framework/lib/src/utils/request_manager.dart Outdated Show resolved Hide resolved
@Leptopoda
Copy link
Member Author

I don't get any linting in my IDEA (not set up correctly) but in android studio it works.
Have you also activated the custom_lint package globally? Maybe run the setup script again.

@Leptopoda
Copy link
Member Author

In a lot of places where errors are caught only the info or warning levels are used, but they should be severe instead. I know that some places can stay info or warning, but most should really be severe.

This is intentional. We did handle the error in the catch clause so just a warning that a process went wrong. I'd only use severe in the global error handler for uncaught errors.
I quite like the graphic in this post https://stackoverflow.com/a/64806781.

@provokateurin
Copy link
Member

Have you also activated the custom_lint package globally? Maybe run the setup script again.

I'm not sure what I did before, but now I ran the setup script, restarted IntelliJ and still nothing.

@provokateurin
Copy link
Member

I quite like the graphic in this post stackoverflow.com/a/64806781.

Can you add this to our docs? I like the graphic as well.

@Leptopoda
Copy link
Member Author

where would this fit?
Maybe a "how to write a client" where we explain package choices and such conventions? Or just a code_style.md?

@Leptopoda Leptopoda force-pushed the feat/neon/logging_framework branch 2 times, most recently from 86d495a to 1c1a656 Compare February 28, 2024 08:30
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

LGTM, just some minor things and you added the requested changes to the wrong commit.

packages/neon_framework/lib/src/bloc/bloc.dart Outdated Show resolved Hide resolved
packages/neon_framework/lib/src/utils/request_manager.dart Outdated Show resolved Hide resolved
packages/neon_framework/lib/src/bloc/bloc.dart Outdated Show resolved Hide resolved
packages/neon_lints/lib/src/avoid_debug_print.dart Outdated Show resolved Hide resolved
@Leptopoda Leptopoda force-pushed the feat/neon/logging_framework branch from 1c1a656 to f6ef506 Compare March 1, 2024 20:05
@Leptopoda Leptopoda requested a review from provokateurin March 1, 2024 20:06
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Thanks a lot ❤️

Can you add the simple test for the lint?

Signed-off-by: Nikolas Rimikis <leptopoda@users.noreply.github.com>
…tion

Signed-off-by: Nikolas Rimikis <leptopoda@users.noreply.github.com>
@Leptopoda Leptopoda force-pushed the feat/neon/logging_framework branch from f6ef506 to 9f5c157 Compare March 2, 2024 13:07
@Leptopoda Leptopoda enabled auto-merge March 2, 2024 13:07
Leptopoda and others added 2 commits March 2, 2024 14:21
Signed-off-by: Nikolas Rimikis <leptopoda@users.noreply.github.com>
…e_fetching_exceptions

Fix/neon_framework/catch_cache_fetching_exceptions
@Leptopoda Leptopoda merged commit ca9eb97 into main Mar 2, 2024
8 checks passed
@Leptopoda Leptopoda deleted the feat/neon/logging_framework branch March 2, 2024 13:51
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