Repository: frictionlessdata/frictionless-py (language: python)

Issue:
`ExcelControl` attribute `stringified` gets lost during `to_copy()`
# Overview

The `stringified` attribute of the `ExcelControl` gets reset to the default value (False) when the control is copied using `to_copy()` (or the resource it is attached to is copied causing it to be copied).  This appears to be because it was missed from the `metadata_profile_patch` definition.

## To reproduce
```python
from frictionless import formats

original = formats.ExcelControl(stringified=True)

copy = original.to_copy()

print("original:", original.stringified, ", copy:", copy.stringified)

# Returns "original: True , copy: False"
```

I'll submit a PR to fix this shortly.

ExcelControl(stringified=True) documentation could clarify interaction with schemas
# Overview

[`formats.excelControl.stringified`](https://framework.frictionlessdata.io/docs/formats/excel.html#reference-formats.excelcontrol.stringified) is documented as "Stringifies all the cell values.".

However it appears that it doesn't actually do that, even in the test cases that were added as part of the PR that added the `stringified` option.

e.g.  in [`def test_xls_parser_cast_int_to_string_1251_xlsx()`](https://github.com/frictionlessdata/frictionless-py/pull/1259/files#diff-8fb31016a58c40a13d350a23e361ffef7761b55a4d1bb8d2a6823d774c1f3a64R131) the test is:

```python
resource = Resource(descriptor, control=formats.ExcelControl(stringified=True))
assert resource.read_rows() == [
    {"A": "001", "B": "b", "C": "1", "D": "a", "E": 1},
    {"A": "002", "B": "c", "C": "1", "D": "1", "E": 1},
]
```

where the `"E": 1` is a number not a string.

Or an even simpler reproduction:

```python
from frictionless import Resource, formats

res = Resource(path="data/table-infer-boolean.xlsx", control=formats.ExcelControl(stringified=True))
print(res.read_rows())
```
prints:
```
[{'number': Decimal('1.0'), 'string': 'two', 'boolean': True}, {'number': Decimal('3.0'), 'string': 'four', 'boolean': False}]
```

Is this the intended behaviour and I am misunderstanding the purpose of `stringified=True` or is this a bug (which I am happy to look at fixing).  

To be honest, the code looks like it should convert everything to strings, but it doesn't seem to according to the unit tests (and our tests). Perhaps @roll or @shashigharti might remember from their work on [the PR that created this capability](https://github.com/frictionlessdata/frictionless-py/pull/1259)?

ExcelControl(stringified=True) documentation could clarify interaction with schemas
# Overview

[`formats.excelControl.stringified`](https://framework.frictionlessdata.io/docs/formats/excel.html#reference-formats.excelcontrol.stringified) is documented as "Stringifies all the cell values.".

However it appears that it doesn't actually do that, even in the test cases that were added as part of the PR that added the `stringified` option.

e.g.  in [`def test_xls_parser_cast_int_to_string_1251_xlsx()`](https://github.com/frictionlessdata/frictionless-py/pull/1259/files#diff-8fb31016a58c40a13d350a23e361ffef7761b55a4d1bb8d2a6823d774c1f3a64R131) the test is:

```python
resource = Resource(descriptor, control=formats.ExcelControl(stringified=True))
assert resource.read_rows() == [
    {"A": "001", "B": "b", "C": "1", "D": "a", "E": 1},
    {"A": "002", "B": "c", "C": "1", "D": "1", "E": 1},
]
```

where the `"E": 1` is a number not a string.

Or an even simpler reproduction:

```python
from frictionless import Resource, formats

res = Resource(path="data/table-infer-boolean.xlsx", control=formats.ExcelControl(stringified=True))
print(res.read_rows())
```
prints:
```
[{'number': Decimal('1.0'), 'string': 'two', 'boolean': True}, {'number': Decimal('3.0'), 'string': 'four', 'boolean': False}]
```

Is this the intended behaviour and I am misunderstanding the purpose of `stringified=True` or is this a bug (which I am happy to look at fixing).  

To be honest, the code looks like it should convert everything to strings, but it doesn't seem to according to the unit tests (and our tests). Perhaps @roll or @shashigharti might remember from their work on [the PR that created this capability](https://github.com/frictionlessdata/frictionless-py/pull/1259)?



Discussion / hints:

With a bit more research, I think I understand what it is doing (though still counter-intuitive).

The `stringified=True` _only_ stringifies the lowest level return from the Excel parser.   Before you get to `read_rows()` or similar, the _schema_ is used to convert whatever is returned by the parser.  If you don't provide a schema, then `detect_schema()` (in `detector.py`) will detect a schema and that will likely pick types other than `string`.

So to actually get an output that is all strings you need _both_ `stringified=True` _and_ a `schema` with all columns set to string types.

So using `table_infer_boolean.xlsx` which looks like:

| / | a | b | c |
|--| ---------|--------| -------- |
|**1**| number | string | boolean |
|**2**| 1|two|true|
|**3**| 3|four|false|

depending on the setting for `stringified` and `schema` we get different output:

| stringified | schema | output | notes |
| -----------| ---------| --------| -------|
| `False` | `None` |  `[{'number': 1, 'string': 'two', 'boolean': True}, {'number': 3, 'string': 'four', 'boolean': False}]` |  Inferred schema, typed output |
| `True` | `None` | `[{'number': 1, 'string': 'two', 'boolean': True}, {'number': 3, 'string': 'four', 'boolean': False}]` |  Inferred schema overrides stringified so still typed output |
| `False` | `{"fields": [{"name": "number", "type": "string"},{"name": "string", "type": "string"},{"name": "boolean", "type": "string"}]}`| `[{'number': None, 'string': 'two', 'boolean': None}, {'number': None, 'string': 'four', 'boolean': None}]` | Without stringified, but with an all-string schema, non-string cells are returned as None |
| `True` |  `{"fields": [{"name": "number", "type": "string"},{"name": "string", "type": "string"},{"name": "boolean", "type": "string"}]}` | `[{'number': '1.0', 'string': 'two', 'boolean': 'True'}, {'number': '3.0', 'string': 'four', 'boolean': 'False'}]` | Both stringified and all-string schema is the only way to get all-string output.  Note though, that the `1` has been converted to `"1.0"` which is not same, so there has clearly been some type conversions even in this case. |

This could probably do with a documentation update to clarify what `stringified` does (and doesn't) do.

With a bit more research, I think I understand what it is doing (though still counter-intuitive).

The `stringified=True` _only_ stringifies the lowest level return from the Excel parser.   Before you get to `read_rows()` or similar, the _schema_ is used to convert whatever is returned by the parser.  If you don't provide a schema, then `detect_schema()` (in `detector.py`) will detect a schema and that will likely pick types other than `string`.

So to actually get an output that is all strings you need _both_ `stringified=True` _and_ a `schema` with all columns set to string types.

So using `table_infer_boolean.xlsx` which looks like:

| / | a | b | c |
|--| ---------|--------| -------- |
|**1**| number | string | boolean |
|**2**| 1|two|true|
|**3**| 3|four|false|

depending on the setting for `stringified` and `schema` we get different output:

| stringified | schema | output | notes |
| -----------| ---------| --------| -------|
| `False` | `None` |  `[{'number': 1, 'string': 'two', 'boolean': True}, {'number': 3, 'string': 'four', 'boolean': False}]` |  Inferred schema, typed output |
| `True` | `None` | `[{'number': 1, 'string': 'two', 'boolean': True}, {'number': 3, 'string': 'four', 'boolean': False}]` |  Inferred schema overrides stringified so still typed output |
| `False` | `{"fields": [{"name": "number", "type": "string"},{"name": "string", "type": "string"},{"name": "boolean", "type": "string"}]}`| `[{'number': None, 'string': 'two', 'boolean': None}, {'number': None, 'string': 'four', 'boolean': None}]` | Without stringified, but with an all-string schema, non-string cells are returned as None |
| `True` |  `{"fields": [{"name": "number", "type": "string"},{"name": "string", "type": "string"},{"name": "boolean", "type": "string"}]}` | `[{'number': '1.0', 'string': 'two', 'boolean': 'True'}, {'number': '3.0', 'string': 'four', 'boolean': 'False'}]` | Both stringified and all-string schema is the only way to get all-string output.  Note though, that the `1` has been converted to `"1.0"` which is not same, so there has clearly been some type conversions even in this case. |

This could probably do with a documentation update to clarify what `stringified` does (and doesn't) do.


Fix patch:
diff --git a/frictionless/formats/excel/control.py b/frictionless/formats/excel/control.py
index 015fc2c9ea..298a2faa3e 100644
--- a/frictionless/formats/excel/control.py
+++ b/frictionless/formats/excel/control.py
@@ -51,6 +51,12 @@ class ExcelControl(Control):
     """
     Stringifies all the cell values. Default value
     is False.
+
+    Note that a table resource schema will still be applied and types coerced to match the schema
+    (either provided or inferred) _after_ the rows are read as strings.
+
+    To return all cells as strings then both set `stringified=True` and specify a 
+    schema that defines all fields to be of type string (see #1659).
     """
 
     # Metadata
@@ -61,5 +67,6 @@ class ExcelControl(Control):
             "fillMergedCells": {"type": "boolean"},
             "preserveFormatting": {"type": "boolean"},
             "adjustFloatingPointError": {"type": "boolean"},
+            "stringified": {"type": "boolean"},
         },
     }


Test patch:
diff --git a/frictionless/formats/excel/__spec__/test_control.py b/frictionless/formats/excel/__spec__/test_control.py
index 27aac29135..a6b2573445 100644
--- a/frictionless/formats/excel/__spec__/test_control.py
+++ b/frictionless/formats/excel/__spec__/test_control.py
@@ -6,3 +6,20 @@
 def test_excel_dialect():
     with Resource("data/table.xlsx") as resource:
         assert isinstance(resource.dialect.get_control("excel"), formats.ExcelControl)
+
+def test_excel_control_to_copy():
+    """
+    Test that the ExcelControl and all its attributes are correctly copied
+    """
+    # Make a control with all values changed from the defaults
+    control_with_changed_attributes = formats.ExcelControl(
+        sheet="non-default",
+        fill_merged_cells=True,
+        preserve_formatting=True,
+        adjust_floating_point_error=True,
+        stringified=True
+    )
+
+    control_copy = control_with_changed_attributes.to_copy()
+
+    assert control_copy == control_with_changed_attributes