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

Refactoring ls and date #7194

Merged
merged 3 commits into from
Jan 22, 2025
Merged

Conversation

Felle33
Copy link
Contributor

@Felle33 Felle33 commented Jan 22, 2025

This PR tries to refactor the common datetime formatting code in ls and date as requested in the issue #7156.
I have created a new file under uucore/lib/features, but I am not sure if it is the right place. Moreover, I tried to import the module but without success, so I left the Cargo.toml of each crate as it was.
Is it correct? How can I import the new module created?

Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@@ -0,0 +1,25 @@
mod timezone_date {
Copy link
Contributor

Choose a reason for hiding this comment

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

please add the license header

/// Get the alphabetic abbreviation of the current timezone.
///
/// For example, "UTC" or "CET" or "PDT".
fn timezone_abbrev() -> &str {
Copy link
Contributor

Choose a reason for hiding this comment

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

please write unit test in this file :)

offset.abbreviation().unwrap_or("UTC").to_string()
}

/// Format the given time according to a custom format string.
Copy link
Contributor

Choose a reason for hiding this comment

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

please document a bit more the arguments as rustdoc

@cakebaker
Copy link
Contributor

I have created a new file under uucore/lib/features, but I am not sure if it is the right place.

Yes, that's the right place.

Moreover, I tried to import the module but without success, so I left the Cargo.toml of each crate as it was.
Is it correct? How can I import the new module created?

You have to wire it up in features.rs, lib.rs, and Cargo.toml of uucore, and then enable the feature in date and ls.

Hope that helps.

@Felle33
Copy link
Contributor Author

Felle33 commented Jan 22, 2025

Thanks for the feedbacks!
I tried to do my best, if you have any suggestion or if there's anything else I need to do, please don't hesitate to let me know.

@Felle33 Felle33 marked this pull request as ready for review January 22, 2025 15:05
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/misc/usage_vs_getopt (passes in this run but fails in the 'main' branch)

@@ -18,13 +18,16 @@ edition = "2021"
path = "src/lib/lib.rs"

[dependencies]
chrono = { workspace = true }
chrono-tz = {workspace = true}
Copy link
Contributor

Choose a reason for hiding this comment

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

A detail that should make the Style/toml job in the CI pass:

Suggested change
chrono-tz = {workspace = true}
chrono-tz = { workspace = true }

@@ -386,7 +360,11 @@ impl TimeStyle {
//So it's not yet implemented
(Self::Locale, true) => time.format("%b %e %H:%M").to_string(),
(Self::Locale, false) => time.format("%b %e %Y").to_string(),
(Self::Format(e), _) => custom_time_format(e, time),
(Self::Format(fmt), _) => {
// this line can be replaced with the one in timzone_date.rs
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this comment is no longer needed?

Suggested change
// this line can be replaced with the one in timzone_date.rs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right

let format_string = &format_string
.replace("%N", "%f")
.replace("%Z", tz_abbreviation.unwrap_or("UTC"));
let format_string = custom_time_format(&format_string);
Copy link
Contributor

Choose a reason for hiding this comment

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

clippy complains about this line:

Suggested change
let format_string = custom_time_format(&format_string);
let format_string = custom_time_format(format_string);

Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/misc/usage_vs_getopt (passes in this run but fails in the 'main' branch)

@cakebaker cakebaker linked an issue Jan 22, 2025 that may be closed by this pull request
@cakebaker cakebaker merged commit 93e3d08 into uutils:main Jan 22, 2025
65 checks passed
@cakebaker
Copy link
Contributor

Thanks!

@Felle33 Felle33 deleted the ls_date_refactor_datetime branch January 22, 2025 16:51
ic3man5 pushed a commit to ic3man5/coreutils that referenced this pull request Jan 22, 2025
* refactoring ls and date

* Refactored and integrated ls and date and added tests to the new feature

* Style refactoring
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.

ls, date: refactor common code for formatting a given datetime
3 participants