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

Job clean refactor #419

Merged
merged 1 commit into from
May 30, 2021

Conversation

davidzwa
Copy link
Contributor

@davidzwa davidzwa commented Apr 7, 2021

This is followup of #406 and will keep track of that first and when that other PR is merged, I will rebase this one on top.

  • Introduce luxon and date formatting
  • Cleanup JobClean (remove typeof undefined, awaits)
  • Test generate(...) and date formatting ✔️ ✔️

@davidzwa davidzwa added fixed on dev This issue has been fixed and is on its way effort-5 labels Apr 7, 2021
@davidzwa davidzwa changed the base branch from master to development April 7, 2021 10:50
@davidzwa davidzwa changed the title Feature/job clean refactor Job clean refactor Apr 7, 2021
@davidzwa davidzwa linked an issue Apr 7, 2021 that may be closed by this pull request
@davidzwa
Copy link
Contributor Author

davidzwa commented Apr 8, 2021

I introduce a certain date format here. I must stress that I see advantage in formatting dates only client side and me as well as Luxon advises against using custom formats. That means just pass ISO date strings or Unix timestamps and let the client figure it out.

This goes further: I prefer not to render HTML server side, not to format numbers and not to postfix (SI) units of measurement (although we could provide the unit of measurements separately). This way the client has the final say in formatting.
This is mainly important for V2, but please take a moment to think about that in your refactoring work.
If we can defer formatting as much as possible, I'd be a big fan.

@davidzwa davidzwa self-assigned this Apr 8, 2021
@davidzwa davidzwa linked an issue Apr 8, 2021 that may be closed by this pull request
@davidzwa davidzwa added the chore label Apr 8, 2021
@davidzwa davidzwa force-pushed the feature/job-clean-refactor branch 10 times, most recently from 1e9ee4e to f194c5f Compare April 14, 2021 17:43
@davidzwa davidzwa marked this pull request as ready for review April 22, 2021 18:34
@davidzwa davidzwa force-pushed the feature/job-clean-refactor branch 8 times, most recently from cdbc3a0 to 341f77e Compare April 27, 2021 13:01
@davidzwa davidzwa force-pushed the feature/job-clean-refactor branch 3 times, most recently from c1210c0 to 2d7893a Compare May 2, 2021 14:09
@NotExpectedYet NotExpectedYet added this to Awaiting Review in 1.3 - The Great Clean up May 3, 2021
@davidzwa davidzwa force-pushed the feature/job-clean-refactor branch 7 times, most recently from 25215c4 to 67469a0 Compare May 11, 2021 07:21
Copy link
Member

@NotExpectedYet NotExpectedYet left a comment

Choose a reason for hiding this comment

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

Some issues I've noticed, apart from these is looking good!

Expected filament costs.

  1. When no spool selected I get Total 0, and Tool 0: null. Total is correct but tool should not be null. We can't really default here regardless so 0 is fine.
  2. Expected Units Total displays 0, even with units on Tool. This is actually a bug of mine so I'm happy to take a look if you'd prefer? I did this like. Been meaning to fix for a while.
  3. Expected printer cost isn't fixed to 2 decimal places. Should be 0.04 on my test gcode and got 0.0429327937923

@NotExpectedYet
Copy link
Member

I introduce a certain date format here. I must stress that I see advantage in formatting dates only client side and me as well as Luxon advises against using custom formats. That means just pass ISO date strings or Unix timestamps and let the client figure it out.

This goes further: I prefer not to render HTML server side, not to format numbers and not to postfix (SI) units of measurement (although we could provide the unit of measurements separately). This way the client has the final say in formatting.
This is mainly important for V2, but please take a moment to think about that in your refactoring work.
If we can defer formatting as much as possible, I'd be a big fan.

Luxon is a sweet package good choice!

Yes I agree there, we can certainly work to push this more client side. Especially for V2.

@davidzwa davidzwa force-pushed the feature/job-clean-refactor branch from f2abe90 to 7ba6bc7 Compare May 11, 2021 20:27
@davidzwa
Copy link
Contributor Author

Lets fix the bugs you mention in this PR. There's breathing room for it, only 5 files changed. I hope to have more PR's like that in future (be honest, History Clean was way to big haha).

@davidzwa davidzwa force-pushed the feature/job-clean-refactor branch 6 times, most recently from d84821a to 6e6af55 Compare May 17, 2021 14:10
@davidzwa
Copy link
Contributor Author

davidzwa commented May 19, 2021

@NotExpectedYet are the bugs you mentioned related to this PR?
The 2nd: sure

Some issues I've noticed, apart from these is looking good!

Expected filament costs.

  1. When no spool selected I get Total 0, and Tool 0: null. Total is correct but tool should not be null. We can't really default here regardless so 0 is fine.

I have no idea what to do haha. What should I look at in the UI or, even better, is this related to this PR?

  1. Expected Units Total displays 0, even with units on Tool. This is actually a bug of mine so I'm happy to take a look if you'd prefer? I did this like. Been meaning to fix for a while.

Please do, because this PR doesnt do a lot except slight code cleanup (no big changes) and date formatting.

  1. Expected printer cost isn't fixed to 2 decimal places. Should be 0.04 on my test gcode and got 0.0429327937923

The third rounding bug is fixed for now server-side.

@davidzwa davidzwa force-pushed the feature/job-clean-refactor branch 2 times, most recently from 156dd29 to 4cc127d Compare May 25, 2021 08:11
@davidzwa davidzwa force-pushed the feature/job-clean-refactor branch from 4cc127d to 1d1a116 Compare May 29, 2021 14:25
Copy link
Member

@NotExpectedYet NotExpectedYet left a comment

Choose a reason for hiding this comment

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

I'll approve this now and then we can fix my bug in another PR.

@davidzwa davidzwa merged commit d3caf08 into OctoFarm:development May 30, 2021
1.3 - The Great Clean up automation moved this from Awaiting Review to Done May 30, 2021
@davidzwa davidzwa deleted the feature/job-clean-refactor branch May 30, 2021 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed on dev This issue has been fixed and is on its way
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

JobClean cleanup DateTime formatting and internationalization
2 participants