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

Remove println on core ns load #370

Closed
wants to merge 1 commit into from
Closed

Remove println on core ns load #370

wants to merge 1 commit into from

Conversation

helins
Copy link
Contributor

@helins helins commented Mar 5, 2023

Libraries printing information without control are often a nuisance. Instead of printing the source of *config*, it is kept in the config itself.

@helins
Remove println on core ns load
395d9db 
Libraries printing information without control are often a nuisance.
Instead of printing the source of `*config*`, it is kept in the config
itself.
@ptaoussanis ptaoussanis self-assigned this Mar 6, 2023
ptaoussanis pushed a commit that referenced this pull request Mar 6, 2023
@helins @ptaoussanis
[new] [#370] Add :_init-config key to *config* map
5647c8d 
Information-only keys, handy for understanding what Timbre's config
was during initial load.
ptaoussanis added a commit that referenced this pull request Mar 6, 2023
@ptaoussanis
[mod] [#370] Remove println output on init load (@helins)
5c1cb01 
Libraries printing information without control are often a nuisance.
@ptaoussanis
Copy link
Member

@AdamClements Hi Adam, thanks for this! Merged, will be included in next release 👍

@ptaoussanis ptaoussanis closed this Mar 6, 2023
ptaoussanis pushed a commit that referenced this pull request Mar 6, 2023
@helins @ptaoussanis
[new] [#370] Add :_init-config map to *config*
90dfc05 
Changes:

  - No longer throw on invalid compile-time :min-level
  - Add `:_init-config` map to `*config*` to help users understand/debug
    load-time config
ptaoussanis added a commit that referenced this pull request Mar 6, 2023
@ptaoussanis
[mod] [#370] Remove println output on init load (@helins)
3ae4764 
Libraries printing information without control are often a nuisance.
@timgilbert
Copy link

timgilbert commented May 10, 2023 •
edited
Loading

Any chance of getting this released onto clojars? The latest release, 6.1.0, doesn't seem to have it.

@ptaoussanis
Copy link
Member

@timgilbert Hi Tim! I do batched work on my libraries, and cut new releases either when there's an important fix or when there's a significant number of changes pending release.

The high-level plan can be found here.

Since Timbre doesn't currently have too many changes pending, it'll probably be at least a few more months before the next release.

Your options in the meantime:

  • I've just pushed the current master to Clojars as an undocumented unstable release, [com.taoensso/timbre "6.2.0-alpha1"].
  • Otherwise would suggest a temporary Timbre fork until the next stable release.

Hope that helps! Cheers :-)

@timgilbert
Copy link

Awesome, thanks @ptaoussanis!

@finalfantasia
Copy link

finalfantasia commented Jun 16, 2023 •
edited
Loading

@ptaoussanis, these unexpected outputs broke our CI/CD pipeline right after an upgrade today (I know, it's finicky and brittle... but priorities and all that...) and it took us a while to realize it was Timbre. It looks like it's been a few months since the last release in Feb, 2023 and there are a few commits pending release. This one looks pretty low risk to me and I'm wondering if it is possible to have a point release (6.1.1?) with it?

As always, we are grateful for and appreciate all your work on Timbre!

Cheers!

@ptaoussanis
Copy link
Member

@finalfantasia Hi Salam,

As always, we are grateful for and appreciate all your work on Timbre!

You're very welcome - I appreciate the kind words.

A couple quick questions:

  1. Just checking that you're aware that [com.taoensso/timbre "6.2.0-alpha1"] is out and includes the change? Is that no good in your case? I understand some places may have policies against pre-release versions. If so, just let me know and I'll try cut a stable release this weekend.

  2. Since you mentioned running into this problem "after an upgrade" - I just want to double check what you mean. A Timbre upgrade, or something else with your system? If a Timbre upgrade, what version were you upgrading from? IIRC, the println output was present in Timbre for quite some time - going back at least a few major versions I believe. Just pointing this out in case there's another issue at work.

Cheers :-)

@finalfantasia
Copy link

finalfantasia commented Jun 20, 2023 •
edited
Loading

Just checking that you're aware that [com.taoensso/timbre "6.2.0-alpha1"] is out and includes the change? Is that no good in your case? I understand some places may have policies against pre-release versions. If so, just let me know and I'll try cut a stable release this weekend.

Yes, I was aware of it when I left my last comment. I think I was being too cautious of using an alpha release in the production environment but this is just my personal stance in general that's not driven by any company policies (that I'm aware of). In order to fix the immediate issue with our CI/CD workflow, we ended up using the alpha version after pair-reviewing all the 6 commits (mostly documentation changes) since the last release of 6.1.0, taking into account the rock solid track record of Timbre, and then determining that there's little to no risk in doing so.

Since you mentioned running into this problem "after an upgrade" - I just want to double check what you mean. A Timbre upgrade, or something else with your system? If a Timbre upgrade, what version were you upgrading from?

I was able to isolate and verify the root cause of the issue to the Timbre upgrade from version 5.2.1 to version 6.1.0.

Again, thanks for your contribution to the Clojure community as well as the greater open-source community.

Cheers!

@ptaoussanis
Copy link
Member

@finalfantasia Hi Salam,

I think I was being too cautious of using an alpha release in the production environment but this is just my personal stance

That's very reasonable, I'd encourage you to continue with that perspective. In this case it just happened to be that that particular alpha is equivalent to the stable point release I would have otherwise pushed.

I was able to isolate and verify the root cause of the issue to the Timbre upgrade from version 5.2.1 to version 6.1.0.

Okay, great 👍 And indeed I was mistaken- the problematic println was only added in v6, I thought it had been there for much longer. Apologies for the trouble that caused!

Again, thanks for your contribution to the Clojure community as well as the greater open-source community.

You're very welcome, thanks for saying so!

Cheers :-)

@ptaoussanis ptaoussanis mentioned this pull request Jun 26, 2023
Closed
@ptaoussanis
Copy link
Member

ptaoussanis commented Jun 26, 2023 •
edited
Loading

Looks like this is causing trouble for Sente. Re-opening until there's a stable release that includes the merged fix.

Will get one out before end of June 👍

@ptaoussanis ptaoussanis reopened this Jun 26, 2023
@ptaoussanis
Copy link
Member

Pushing a new stable release of Timbre shortly, marking as done.

ptaoussanis pushed a commit that referenced this pull request Jun 30, 2023
@helins @ptaoussanis
[new] [#370] Add :_init-config map to *config*
a0bc5e0 
Changes:

  - No longer throw on invalid compile-time :min-level
  - Add `:_init-config` map to `*config*` to help users understand/debug
    load-time config
ptaoussanis added a commit that referenced this pull request Jun 30, 2023
@ptaoussanis
[fix] [#370] Remove println output on init load (@helins)
5c18945 
Libraries printing information without control are often a nuisance.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ptaoussanis ptaoussanis

Labels
bug enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@helins @ptaoussanis @timgilbert @finalfantasia