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

custom timestamp #232

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

custom timestamp #232

wants to merge 11 commits into from

Conversation

abdelaziz-mahdy
Copy link
Contributor

@abdelaziz-mahdy abdelaziz-mahdy commented May 13, 2024

initial work to allow this change #231

  • added new time format and enum to pick it
  • make the new time format choice available in settings and passed everywhere it should be used
  • maybe add custom time output (where people can make a custom function that takes time and returns string that should be used)
  • update tests

@abdelaziz-mahdy
Copy link
Contributor Author

if the custom time function should be the way to go let me know,
in that case i think it will be more customizable but keeping both options is better to have an easy choice and a fully custom one

@Frezyx
Copy link
Owner

Frezyx commented May 26, 2024

Hello @abdelaziz-mahdy ! Sorry for the delay 🙏🏻
This is a very important feature for the library, but I have suggestions for how it could be implemented.
Can we provide TimeFormat or formatting method as generateMessage method named (optional) argument ?

Example of code below:

  String generateTextMessage({TimeFormat timeFormat = TimeFormat.timeAndSeconds}) {
    return '$<formatted time by method>$displayTitle$message$displayStackTrace';
  }

This implementation resolve some issues:

  1. Providing TimeFormat for all TalkerData instances is not very clear logic.
  2. We can change the log format during the application's operation, but it won't be applied to all created records in history.
  3. TalkerHistory saving in runtime in application memory. This field adding more unused data for heap.

In this case (providing timeFormat as method arg) we can setup it in TalkerSettings in package initialization one time.
What you think about that ?

@abdelaziz-mahdy
Copy link
Contributor Author

I like that idea, since changing all of the methods was alot of work and didn't think it will be a clean approach, will check it and let you know

@abdelaziz-mahdy
Copy link
Contributor Author

my only problem is that i dont know how to pass it to settings and use it, right now my changes allow to use any time format since i added the param to the TalkerData but the point of passing the time format to it is the main problem for me

@abdelaziz-mahdy
Copy link
Contributor Author

abdelaziz-mahdy commented May 26, 2024

using your idea i think we will have a problem with this

extension TalkerDataInterfaceListExt on List<TalkerData> {
  /// The method allows you to get
  /// full text of logs or history
  String get text {
    final sb = StringBuffer();
    for (final data in this) {
      sb.write('${data.generateTextMessage()}\n');
    }
    return sb.toString();
  }
}

where my previous approach didnt have a problem with it

well, i fixed it ignore this.

@abdelaziz-mahdy abdelaziz-mahdy marked this pull request as ready for review May 26, 2024 21:19
@codecov-commenter
Copy link

codecov-commenter commented May 26, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 98.33%. Comparing base (9537000) to head (506deea).

Files Patch % Lines
packages/talker/lib/src/utils/time_formatter.dart 45.45% 6 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #232      +/-   ##
==========================================
- Coverage   99.82%   98.33%   -1.49%     
==========================================
  Files          28       17      -11     
  Lines         565      421     -144     
==========================================
- Hits          564      414     -150     
- Misses          1        7       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@abdelaziz-mahdy
Copy link
Contributor Author

i think its now ready for a review, let me know what should be added or changed, or if its all good

Copy link
Owner

@Frezyx Frezyx left a comment

Choose a reason for hiding this comment

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

Everything is great in code implementation 👍🏻
I will try to make some changes to support backward compatibility.

@Frezyx Frezyx added enhancement New feature or request work_in_progress Сurrently under work labels May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request work_in_progress Сurrently under work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support new TimeFormat in ecosystem packages Add date to log
3 participants