refactor: excel2json (DEV-2547) #487
Conversation
Creation of folder and changes of import paths.
Cleaning Datatype
Removal of unnecessary statements.
The compliance checks for the files were changed and testing added. To this end several generic functions were created and tested. The creation and validation of the json string remains wip.
DEV-2547 Refactoring DSP-TOOLS excel_to_json
Reorganization of the folder structure to create a new folder only for excel_to_json. Refactoring and addition of testing the excel_to_json_properties file. |
Nora-Olivia-Ammann
changed the title
Nora-Olivia-Ammann
changed the title
Nora-Olivia-Ammann
changed the title
# Conflicts: # test/unittests/test_excel_to_json_properties.py # testdata/excel2json/excel2json_files/test-name (test_label)/properties.xlsx
…//github.com/dasch-swiss/dsp-tools into DEV-2547-Refactoring-DSP-TOOLS-excel_to_json
| return {"wrong gui_attributes": final_series} | ||
|
|
||
|
|
||
| def _check_missing_values_in_row_raise_error(to_check_df: pd.DataFrame, excel_filename: str) -> None: |
There was a problem hiding this comment.
Can you name this function in a way that tells the user what this function does? In some other comments, I gave you suggestions how to rename other functions.
There was a problem hiding this comment.
What is it here that is unclear?
There was a problem hiding this comment.
"Clean code reads like well-written prose." (Robert C. Martin)
If we look at it from the caller side: When we use a function, we want to know what it does. Some examples from my suggestions in other comments:
| your name | my suggestion |
|---|---|
| check_duplicate_raise_error() | check_column_for_duplicates() |
| _search_validation_error() | _prepare_err_msg_from_json_validation_error |
| check_required_columns_raise_error() | check_that_df_contains_required_columns() |
The reason I prefer my suggestions is that it's almost like telling the computer: "do that for me". So the reason I struggle with your name _check_missing_values_in_row_raise_error() is that I cannot translate it into a spoken command like "Hey computer, please check the missing values in the row and raise an error".
See also: https://dev.to/pksilen/27-tips-for-making-your-code-read-like-beautifully-written-prose-4aab
There was a problem hiding this comment.
Tip #6: Use a preposition in the function name only when necessary: You don’t need to add a preposition to a function name if the preposition can be assumed (i.e., the preposition is implicit).
?
There was a problem hiding this comment.
Would this not violate this rule?
There was a problem hiding this comment.
Do you mean that Tip no. 6 from dev.to violates Robert Martin's quote? These rules are not natural laws, rather they point you in a certain direction.
So in the present case (discussing about _check_missing_values_in_row_raise_error), it's too far away from natural language. Especially, I struggle with the _raise_error part. If you delete that, it would already be better.
Generally: You have several function names that end in raise_error. As far as I can tell, it's quite uncommon to include that in a function name. It would be better to do it like you did in _check_compliance_gui_attributes(): There, the docstring says that the function might raise an error, but you didn't pack this info into the function name.
| """ | ||
| This function takes a pd.Dataframe and a list of column names which may not contain empty cells. If there are any | ||
| empty cells in the column, it adds the column name and a boolean pd.Series to the dictionary. If there are no empty | ||
| cells, then it is not included in the dictionary. If no column has any empty cells, then it returns an empty |
There was a problem hiding this comment.
A general feedback to your docstrings: I think that you should make a conceptual distinction between inline comments and docstrings:
- Inline comments explain implementation details. They are intended at other developers.
- Docstring are 1 abstraction level higher: They are intended at users of the function. A docstring should enable the user to decide if he needs this function or not. It should explain to the outside world what tasks can be accomplished with this function.
There was a problem hiding this comment.
This PR is a big improvement of the code. I especially like the modular approach.
However, there are some practical things, some "handwerkliche Details", that should be improved, especially the naming of the functions. In my comments, I gave some examples how the functions could be named.
Can you in a first step improve the naming of the functions? In a second step, I could then review again. Because at the moment, i find it hard to review, because I find it difficult to understand what the functions do.
Co-authored-by: Johannes Nussbaum <39048939+jnussbaum@users.noreply.github.com>
…//github.com/dasch-swiss/dsp-tools into DEV-2547-Refactoring-DSP-TOOLS-excel_to_json
| Based on the string content of the individual rows, which is specified in the "substring_list", | ||
| the cell is the column "check_empty_colname" is checked whether it is empty or not. | ||
| The "substring_list" contains the different possibilities regarding the content of the cell. | ||
| They are joined in a RegEx "|" which denotes "or". |
There was a problem hiding this comment.
The docstring of a function is not the right place to explain regex syntax. This is an implementation detail that doesn't matter to the person who uses this function.
| for file in os.listdir("testdata/tmp"): | ||
| os.remove("testdata/tmp/" + file) | ||
| os.rmdir("testdata/tmp") |
There was a problem hiding this comment.
These 3 lines can be simplified to shutil.rmtree("testdata/tmp")
Nora-Olivia-Ammann
deleted the
DEV-2547-Refactoring-DSP-TOOLS-excel_to_json
branch
No description provided.