Fix #4157: handle error in case of unsupported format #4474

itsankitkp · 2023-01-27T19:00:47Z

Chrono panics in case of unsupported formats, this patch handles such cases and returns supplied format as a result.

fanninpm · 2023-01-27T19:41:45Z

Lib/test/test_time.py

+        # XXX this test case fails, this should have been handled in
+        # in in layers above fn strftime.
+        # self.assertRaises(ValueError, time.strftime, '%S\0', tt)
+
+        # check if unsupported arg in strftime returns arg itself
+        self.assertEqual(time.strftime("%4Y"), "%4Y")


Is this in CPython?

No, CPython returns '2023'. But rust chrono doesn't support it yet

We prefer to keep the tests in the Lib/test directory as close as possible to what's in CPython.

Okay, should I keep it in extra_tests/snippets/stdlib_datetime.py ?

If the "%S\0" case no longer panics, then you could change the decorator to this:

# TODO: RUSTPYTHON @unittest.expectedFailure def test_function(): # ...

If the "%4Y" case no longer panics, then you could change the decorator to this:

# TODO: RUSTPYTHON @unittest.expectedFailure def test_function(): # ...

I have done this, test ran fine.

fanninpm · 2023-01-27T19:54:41Z

vm/src/stdlib/time.rs

+
+        let mut formatted_time = String::new();
+
+        /*
+         * chrono doesn't support all formats and it
+         * raised an error if unsupported format is supplied.
+         * If error happens, we set result as input arg
+         */
+        let result = write!(&mut formatted_time, "{}", instant.format(format.as_str()));
+        match result {
+            Ok(r) => r,
+            Err(_) => formatted_time = format.to_string(),
+        };


Does this work?

Suggested change

let mut formatted_time = String::new();

/*

* chrono doesn't support all formats and it

* raised an error if unsupported format is supplied.

* If error happens, we set result as input arg

*/

let result = write!(&mut formatted_time, "{}", instant.format(format.as_str()));

match result {

Ok(r) => r,

Err(_) => formatted_time = format.to_string(),

};

let formatted_time = String::try_from(instant.format(format.as_str())).unwrap_or(|| format.to_string());

I am getting following error

the trait std::convert::From<DelayedFormat<StrftimeItems<'_>>> is not implemented for std::string::String

But I do see what you are trying to do here. I will give another pass at this.

I have refactored code, it looks lot better now

fanninpm · 2023-01-27T19:56:32Z

It's fine to have that "%4Y" case in the extra_tests/.

itsankitkp · 2023-01-27T20:28:47Z

It's fine to have that "%4Y" case in the extra_tests/.

Okay, I tried that but there is an issue
If I write assert_equal(_time.strftime("%4Y"), "%4Y"),
it runs first arg against cpython (which returns "2023") and 2nd part against rustpython (which returns "%4Y")
What I require is to run both first and second argument against rustpython only. Am I looking at wrong direction here?

Assertion Failure: <class 'str'>(2023) == <class 'str'>(%4Y)

itsankitkp · 2023-01-31T16:11:03Z

It's fine to have that "%4Y" case in the extra_tests/.

Okay, I tried that but there is an issue If I write assert_equal(_time.strftime("%4Y"), "%4Y"), it runs first arg against cpython (which returns "2023") and 2nd part against rustpython (which returns "%4Y") What I require is to run both first and second argument against rustpython only. Am I looking at wrong direction here?

Assertion Failure: <class 'str'>(2023) == <class 'str'>(%4Y)

I have fixed this for now by using unsupported format which returns same result in both python and rustpython.

youknowone · 2023-02-17T12:35:34Z

@itsankitkp it passes tests on ubuntu, but not on macos and windows. could you check it?

----------------------------- Captured stdout call -----------------------------
Assertion Failure: <class 'str'>(?) == <class 'str'>(%?)
----------------------------- Captured stderr call -----------------------------
Traceback (most recent call last):
  File "/Users/runner/work/RustPython/RustPython/extra_tests/snippets/stdlib_datetime.py", line 135, in <module>
    assert_equal(_time.strftime("%?"), "%?")
  File "/Users/runner/work/RustPython/RustPython/extra_tests/snippets/testutils.py", line 58, in assert_equal
    _assert_print(lambda: a == b, [_typed(a), '==', _typed(b)])
  File "/Users/runner/work/RustPython/RustPython/extra_tests/snippets/testutils.py", line 47, in _assert_print
    assert f()
AssertionError

Bumps [openssl-src](https://github.com/alexcrichton/openssl-src-rs) from 111.24.0+1.1.1s to 111.25.0+1.1.1t. - [Release notes](https://github.com/alexcrichton/openssl-src-rs/releases) - [Commits](https://github.com/alexcrichton/openssl-src-rs/commits) --- updated-dependencies: - dependency-name: openssl-src dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>

Closes RustPython#4501

Chrono panics in case of unsupported formats, this patch handles such cases and returns supplied format as a result.

youknowone · 2023-02-20T06:29:52Z

rebase seems messed up. could you try:

git fetch upstream
git rebase upstream/main

when upstream is github.com/RustPython/RustPython

itsankitkp · 2023-02-20T15:15:25Z

rebase seems messed up. could you try:
git fetch upstream
git rebase upstream/main
when upstream is github.com/RustPython/RustPython

This branch was messed up, I have created fresh PR with same changes with history fixed.
Can you please take a look #4530

itsankitkp · 2023-02-20T15:15:51Z

Closing as I have opened other PR: #4530 with fixed git history

fanninpm requested changes Jan 27, 2023

View reviewed changes

itsankitkp mentioned this pull request Jan 27, 2023

time.strftime(arg) fails when arg is not valid format string #4157

Open

itsankitkp changed the title ~~handle error in case of unsupported format~~ Fix #4157: handle error in case of unsupported format Jan 27, 2023

itsankitkp requested a review from fanninpm January 27, 2023 19:47

fanninpm reviewed Jan 27, 2023

View reviewed changes

DimitrisJim linked an issue Jan 30, 2023 that may be closed by this pull request

time.strftime(arg) fails when arg is not valid format string #4157

Open

itsankitkp force-pushed the handle-panic-strftime branch from 0cf0b04 to d4c2fd3 Compare January 31, 2023 15:30

itsankitkp requested a review from fanninpm January 31, 2023 15:31

itsankitkp force-pushed the handle-panic-strftime branch from fd2f370 to 052e984 Compare February 18, 2023 08:30

DimitrisJim and others added 17 commits February 19, 2023 11:06

Fix clippy issues for rust 1.67 (RustPython#4478)

eb5dd55

feat: allow not set SIGINT handler

1d0cb29

feat: impl Detail option for not set SIG_INT

844ff75

use qualname in TypeErrors for functions (RustPython#4476)

3a08ff1

Match on ascii start/continuation characters before calling functions.

29e322b

Don't call is_emoji_presentation for each invocation of consume_normal

516c41d

Add initial capacities, use u32s for indents/spaces.

a3eb988

Eat for comma.

6ba8191

Hint that the unwrap should always succeed.

4c37307

use workspace dependencies

2cb6634

Move NewLineHandler inline, don't check each character twice.

01380bf

Document lexer.

8ef74d6

Try to fix mac build

0d3ff4d

update libffi

c682063

bump up openssl and libffi

1f9a48f

skip run rust tests for macOS CI

600a3da

DimitrisJim and others added 23 commits February 19, 2023 11:06

Add tests, some comments, to function.rs.

e53e891

extra_tests/snippets/{builtins => builtin_eval}.py

9fc54fd

remove duplicated tests from tests/stdlib_math.py

a8d63ff

Remove completed TODO

1b6d45d

wrap_index without abs

6871964

optimize str.(l|r)strip

6731150

Fix aarch64 compatibility for sqlite.

0c7324e

Fix str.join with str subclass

c45bcfa

Add co_freevars to code object

96178a1

Fix code linting

9ac6e99

Add co_cellvars to code object

c5a629b

Closes RustPython#4501

Fix unexpected success in test_future.

1755f04

Use nix for more things

390c8da

Tidy up ssl a little

8bac26f

Run cargo-update

39ddc50

PyObjectRef::downcast_exact returns PyRefExact

59c7536

Add description for PyRefExact

c97d504

Optimize Py<PyDict>::to_attributes() to reuse PyRefExact

ef5ba76

Optimize bytes-like (l|r)strip (RustPython#4500)

b1f41c1

narrow publicity of BytesInner::elements

900f49a

remove unnecessary to_vec()

ad0b15e

Handle panic in case of unsupported format in chrono

5b17bd6

Chrono panics in case of unsupported formats, this patch handles such cases and returns supplied format as a result.

feat! add test case for unsupported time format

d4a6d39

itsankitkp force-pushed the handle-panic-strftime branch from be812d7 to d4a6d39 Compare February 19, 2023 05:37

itsankitkp closed this Feb 20, 2023

fanninpm mentioned this pull request Feb 23, 2023

Update test.support from CPython 3.11.2 #4537

Merged

5 tasks

youknowone mentioned this pull request Feb 23, 2023

Return arg in case of invalid param in strftime #4530

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4157: handle error in case of unsupported format #4474

Fix #4157: handle error in case of unsupported format #4474

itsankitkp commented Jan 27, 2023

fanninpm Jan 27, 2023

itsankitkp Jan 27, 2023

fanninpm Jan 27, 2023

itsankitkp Jan 27, 2023

fanninpm Jan 27, 2023 •

edited

Loading

itsankitkp Jan 27, 2023

fanninpm Jan 27, 2023

itsankitkp Jan 27, 2023 •

edited

Loading

itsankitkp Jan 27, 2023

itsankitkp Jan 31, 2023

fanninpm commented Jan 27, 2023

itsankitkp commented Jan 27, 2023

itsankitkp commented Jan 31, 2023

youknowone commented Feb 17, 2023

youknowone commented Feb 20, 2023

itsankitkp commented Feb 20, 2023

itsankitkp commented Feb 20, 2023

Fix #4157: handle error in case of unsupported format #4474

Fix #4157: handle error in case of unsupported format #4474

Conversation

itsankitkp commented Jan 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanninpm Jan 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsankitkp Jan 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanninpm commented Jan 27, 2023

itsankitkp commented Jan 27, 2023

itsankitkp commented Jan 31, 2023

youknowone commented Feb 17, 2023

youknowone commented Feb 20, 2023

itsankitkp commented Feb 20, 2023

itsankitkp commented Feb 20, 2023

fanninpm Jan 27, 2023 •

edited

Loading

itsankitkp Jan 27, 2023 •

edited

Loading