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

Support custom formatting - especially allow easy TZ support 😄 #226

Merged
merged 3 commits into from
Feb 1, 2022

Conversation

alaendle
Copy link
Member

@alaendle alaendle commented Dec 2, 2021

Arises from #217. As a first commit, just the basic changes to reduce code duplication.

@alaendle
Copy link
Member Author

alaendle commented Dec 4, 2021

Oh dear, the CI has definitely not been running for too long 😢😉

@chshersh
Copy link
Contributor

chshersh commented Dec 4, 2021

@alaendle I'll update CI in a separate PR as well where I'll move co-log out and remove other packages. This package hasn't been updated for a while and tools become outdated too quickly unfortunately 😞

@alaendle
Copy link
Member Author

alaendle commented Dec 7, 2021

@chshersh One of my weaknesses, I'm impatient 😉. Could you please once more approve the start of the workflow? My hope is that the latest ubuntu image contains the missing lib (since the ci from co-log-core seems to work). I just see it as an intellectual challenge to get this up and running 🤦‍♂️. And even with https://github.com/nektos/act it's hard to foresee how the actual github runner will behave.

@chshersh
Copy link
Contributor

chshersh commented Dec 7, 2021

@alaendle I gave Write access to this repository. I believe, if you open PRs next time, you won't need other maintainers' approval to run CI 🙂

@chshersh chshersh added enhancement New feature or request refactoring labels Dec 7, 2021
@alaendle
Copy link
Member Author

alaendle commented Dec 8, 2021

Wow thanks! CI build is running, so maybe it's now time for feedback for the change and concentrate on the improvement of the intended change. Thanks again!

@alaendle
Copy link
Member Author

Happy New Year @chshersh, I don't want to push, but is there any way to get ahead? Any obstacles that I could resolve?

Comment on lines +506 to +507
>>> showThreadId <$> Control.Concurrent.myThreadId
"[ThreadId 4898] "
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid this test won't work because ThreadId is non-deterministic and could be different each time 😞
But I wonder how tests are passing here on multiple CIs 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems co-log doesn't run doctest - I'll take a look what changes are required to add it.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #228

Copy link
Contributor

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

@alaendle The PR looks good 👍🏻

I was currently teaching the Haskell course so I wasn't around to do any work. But I'm still planning to refactor this repository and bootstrap the co-log org.

@chshersh chshersh merged commit 95b3fe6 into co-log:main Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants