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

[PR] Fixing timer string not updating after editing timer object #246

Merged
merged 7 commits into from
Jan 17, 2023

Conversation

LuchoTurtle
Copy link
Member

fix #244

Item timer should now update accordingly.

@LuchoTurtle LuchoTurtle added bug Suspected or confirmed bug (defect) in the code in-progress An issue or pull request that is being worked on by the assigned person labels Jan 4, 2023
@LuchoTurtle LuchoTurtle self-assigned this Jan 4, 2023
@codecov
Copy link

codecov bot commented Jan 4, 2023

Codecov Report

Merging #246 (33b5eb5) into main (9d57c7f) will increase coverage by 9.34%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main      #246      +/-   ##
===========================================
+ Coverage   90.65%   100.00%   +9.34%     
===========================================
  Files          15        14       -1     
  Lines         428       390      -38     
===========================================
+ Hits          388       390       +2     
+ Misses         40         0      -40     
Impacted Files Coverage Δ
lib/app_web/live/app_live.ex 100.00% <100.00%> (+0.83%) ⬆️

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

@LuchoTurtle
Copy link
Member Author

LuchoTurtle commented Jan 4, 2023

This line that is not covered is frustrating.

It clearly is covered by test/app_web/live/app_live_test.exs:522 (the diff is 15642000) but it fails because of formatting issues.

If I change the line in timer_text (the function where 'one line is not being covered') to

      s = if diff > 1000 do
          s = (diff / 1000) |> trunc()
          s = if s > 60, do: Integer.mod(s, 60), else: s
          left_pad(s)

the coverage is 100%.

But since the Github Action mandates the lines to be formatted, this line keeps popping off as "not covered" (and inconsistently, which is even more frustrating).

:(

@LuchoTurtle
Copy link
Member Author

LuchoTurtle commented Jan 4, 2023

@nelsonic have you ever had this happen to you?
Having trouble circumventing this issue :/

@nelsonic
Copy link
Member

nelsonic commented Jan 5, 2023

Not had that issue before. Does it pass on your localhost? 💭

@LuchoTurtle
Copy link
Member Author

Yes, it does.
It's an issue with how the formatted changes the if statement to a different line, coveralls doesn't "catch it", even though tests definitively cover it.

@nelsonic
Copy link
Member

@SimonLab can you take a quick look at this? 🙏

@LuchoTurtle
Copy link
Member Author

@nelsonic this PR should now be mergeable.

@LuchoTurtle LuchoTurtle assigned nelsonic and unassigned LuchoTurtle Jan 17, 2023
@LuchoTurtle LuchoTurtle added awaiting-review An issue or pull request that needs to be reviewed and removed in-progress An issue or pull request that is being worked on by the assigned person labels Jan 17, 2023
BUILDIT.md Outdated Show resolved Hide resolved
Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

Thanks @LuchoTurtle 👍

Sadly the review app: https://mvp-pr-246.fly.dev/ is erroring:
image

But the code looks good and tests pass. 👌

@nelsonic nelsonic merged commit 4cd62f9 into main Jan 17, 2023
@nelsonic nelsonic deleted the fix-#244 branch January 17, 2023 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review An issue or pull request that needs to be reviewed bug Suspected or confirmed bug (defect) in the code
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Chore: Timers Don't Update When Start/Stop time is updated.
2 participants