Skip to content

Conversation

@iamgabrielsoft
Copy link
Contributor

This PR is tied to issue #3. Take a look for more information

@iamgabrielsoft
Copy link
Contributor Author

@dr-orlovsky please review

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

Thank you very much for the excellent work! I am quite happy with it; have only two minor comments.

PS Sorry for the late review; was on a conference last week and didn't have a chance to look properly.

#[repr(u8)]
#[derive(Clone, PartialEq, Eq, Debug)]
pub enum LogLevel {
/// Do not log anything. Corresponds to zero verbosity flags.
Copy link
Member

Choose a reason for hiding this comment

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

Why are we removing doc comments here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was not intended, please it will be added back in the next commit

.help("Increase verbosity level (e.g., -vvv for Info)"),
)
.arg(
Arg::new("json")
Copy link
Member

Choose a reason for hiding this comment

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

Here and below: since this is a library, its clients may use names json and other in their own command-line arguments. Also, it is not clear why these names are related to logs.

I propose to add prefix log- to all these elements we introduce here (except verbose).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

other prefix was also added too, the reason is to format the log response base on the prefix passed, like child_logger, json etc

@iamgabrielsoft
Copy link
Contributor Author

@dr-orlovsky can the rustup version be increased? some sub-pkg are failing on building in toolchain

@dr-orlovsky
Copy link
Member

Sure, you can bump MSRV

@iamgabrielsoft
Copy link
Contributor Author

iamgabrielsoft commented Jul 13, 2025

@dr-orlovsky i increased the rust version for the lib, because some transitive dependency are breaking. Please look into the review

@iamgabrielsoft
Copy link
Contributor Author

@dr-orlovsky review

@iamgabrielsoft
Copy link
Contributor Author

@dr-orlovsky this is waiting for your review

@dr-orlovsky
Copy link
Member

Hi and sorry for the long delay. I took off from FOSS and GitHub due to a very negative experience I had. Now kinda back

@dr-orlovsky dr-orlovsky merged commit f577da9 into rust-amplify:master Oct 15, 2025
13 of 14 checks passed
@iamgabrielsoft
Copy link
Contributor Author

I understand, me too.
@dr-orlovsky sorry to hear that, is it something you wish to talk about privately?
Am available on several platforms, which one do you prefer?

@iamgabrielsoft
Copy link
Contributor Author

iamgabrielsoft commented Oct 15, 2025

Also what's next for this library, would love to hear?

I think I don't have your mail, communication would be faster

@dr-orlovsky
Copy link
Member

Are you on Telegram? Also the e-mail is dr at orlovsky ch.
I also wish you to add to the maintainers of the library. Are you ok with that?

@iamgabrielsoft
Copy link
Contributor Author

Yes am on telegram. My username is @iamgabrielsoft.

My email is also: [email protected]

Yes am okay accepting to be a maintainer.

@iamgabrielsoft
Copy link
Contributor Author

Please give me your telegram username so I can add you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants