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

Added success/failure callback hooks to Authenticator. #527

Merged
merged 6 commits into from
Oct 26, 2023
Merged

Added success/failure callback hooks to Authenticator. #527

merged 6 commits into from
Oct 26, 2023

Conversation

diegotori
Copy link
Contributor

Addresses Issue #526.

@techouse
Copy link
Collaborator

Can you please fix the linter issues?

@techouse techouse self-assigned this Oct 26, 2023
@techouse techouse added the enhancement New feature or request label Oct 26, 2023
@diegotori
Copy link
Contributor Author

@techouse fixed. Please try running the workflow again.

@codecov
Copy link

codecov bot commented Oct 26, 2023 •
edited
Loading

Codecov Report

Merging #527 (c8e5c2f) into develop (f061355) will increase coverage by 0.07%.
Report is 1 commits behind head on develop.
The diff coverage is 66.66%.

@@             Coverage Diff             @@
##           develop     #527      +/-   ##
===========================================
+ Coverage    93.95%   94.02%   +0.07%     
===========================================
  Files            9       10       +1     
  Lines          463      469       +6     
===========================================
+ Hits           435      441       +6     
  Misses          28       28
Files Coverage Δ
chopper/lib/src/base.dart 96.29% <100.00%> (+1.63%) ⬆️
chopper/lib/src/authenticator.dart 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@diegotori
Copy link
Contributor Author

@techouse looks like codecov is complaining about classes that I've already added tests for.

@diegotori
PR lejard-h#527. Ignoring coverage on new Authenticator hook methods,…
4f49a00 
… since tests were added to cover the new code.
@techouse
Copy link
Collaborator

@techouse looks like codecov is complaining about classes that I've already added tests for.

What do you mean? The coverage went up. Can you please remove the coverage exceptions?

@diegotori
Copy link
Contributor Author

@techouse looks like codecov is complaining about classes that I've already added tests for.

What do you mean? The coverage went up. Can you please remove the coverage exceptions?

Let the automation finish, since it failed on the codecov/patch job, due it complaining that I had no tests covering the new Authenticator hooks.

This one so far has passed codecov. If you want it gone, then I can easily remove. Right now, all the checks are green.

techouse
techouse requested changes Oct 26, 2023
View reviewed changes
chopper/lib/src/authenticator.dart Outdated Show resolved Hide resolved
chopper/lib/src/authenticator.dart Outdated Show resolved Hide resolved
@diegotori
Copy link
Contributor Author

@techouse here's the relevant codecov job that failed.

@techouse
Copy link
Collaborator

@techouse here's the relevant codecov job that failed.

You can ignore this. I'd rather have codecov complain than silenced, so please remove the ignore lines.

techouse
techouse requested changes Oct 26, 2023
View reviewed changes
Copy link
Collaborator

@techouse techouse left a comment

Choose a reason for hiding this comment

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

I don't like the empty methods in the abstract. Can you change them to a getter syntax?

chopper/lib/src/authenticator.dart Outdated Show resolved Hide resolved
chopper/lib/src/base.dart Outdated Show resolved Hide resolved
chopper/lib/src/base.dart Outdated Show resolved Hide resolved
chopper/test/fake_authenticator.dart Outdated Show resolved Hide resolved
chopper/test/fake_authenticator.dart Outdated Show resolved Hide resolved
@techouse
Copy link
Collaborator

I'm not really sure why the coverage is not picking this up. Anyhow, just fix the documentation of the Authenticator class and we should be good to merge this :)

@techouse techouse merged commit e34feea into lejard-h:develop Oct 26, 2023
5 of 6 checks passed
@techouse
Copy link
Collaborator

Thank you for your contribution ♥️

@diegotori diegotori deleted the authenticator_callback_hooks branch October 26, 2023 20:37
techouse added a commit that referenced this pull request Oct 29, 2023
@techouse
🔖 release chopper v7.0.9
d84b1f6 
# chopper

## 7.0.9

- #527
- #529
@techouse techouse mentioned this pull request Oct 29, 2023
Merged
techouse added a commit that referenced this pull request Oct 30, 2023
@techouse @diegotori
🔖 release chopper v7.0.9 (#531)
9f3b2e7 
# chopper

## 7.0.9

- #527
- #529

---------

Co-authored-by: Diego Tori <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@techouse techouse techouse approved these changes

@JEuler JEuler Awaiting requested review from JEuler JEuler is a code owner

@lejard-h lejard-h Awaiting requested review from lejard-h lejard-h is a code owner

@meysam1717 meysam1717 Awaiting requested review from meysam1717 meysam1717 is a code owner

@stewemetal stewemetal Awaiting requested review from stewemetal stewemetal is a code owner

Assignees

@techouse techouse

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@diegotori @techouse