Skip to content

Excel to entities #56

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 20 commits into from
Jan 28, 2025
Merged

Excel to entities #56

merged 20 commits into from
Jan 28, 2025

Conversation

carlosmada22
Copy link
Collaborator

No description provided.

@carlosmada22 carlosmada22 linked an issue Jan 22, 2025 that may be closed by this pull request
Define overwrite and update for excel import-export #45
Closed
JosePizarro3
JosePizarro3 reviewed Jan 23, 2025
View reviewed changes
Copy link
Member

@JosePizarro3 JosePizarro3 left a comment

Choose a reason for hiding this comment

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

Very nice! can you add some testing and some excels where to do this?

Also, I added a few minor comments on minor design this you can decide if accept or just keep doing it with your style.

Then, one open point: some code doing checks on the format is commented out. We would actually like to keep this, right? Is this the issue you were having with the logger?

bam_masterdata/cli/excel_to_entities.py
"Section",
"Property label",
"Data type",
"Vocabulary code",
Copy link
Member

Choose a reason for hiding this comment

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

Can you add "Object code" commented out? This is because in the excel import system is missing, but for our Python and other stuff we need this information when the Data type is OBJECT

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added, but you mean to have it there for future versions of the Excel files that will contain also the header "Object code"?

Copy link
Member

Choose a reason for hiding this comment

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

well, now I am not so sure; this is something internal from openBIS, but it looks like a bug to me: we need to somehow define the code of the object for when a property is of dataType OBJECT

Otherwise, how do we know the object that an assigned property is referring to?

bam_masterdata/cli/excel_to_entities.py
entity_type = sheet[entity_type_position].value

entity_types = [
"OBJECT_TYPE",
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need both OBJECT_TYPE and SAMPLE_TYPE here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because right now the entity types of type "OBJECT" appear in the excel as "SAMPLE_TYPE". This naming was deprecated in favour of "OBJECT_TYPE", but until this happens, I think is nice to allow the system to recognize both, until we have a definitive version, and is not changing the behaviour of the script in any way.

bam_masterdata/cli/excel_to_entities.py
)
# return "\n".join(errors)
else:
if entity_type == "SAMPLE_TYPE" or entity_type == "OBJECT_TYPE":
Copy link
Member

Choose a reason for hiding this comment

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

for readability, this and the previous ifs in the terms_to_dict could be part of a class and methods applied to each type. There you can identify common abstracted methods to be applied

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can discuss this

@coveralls
Copy link

coveralls commented Jan 24, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 13010536952

Details

  • 18 of 360 (5.0%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-1.9%) to 95.421%
Changes Missing Coverage Covered Lines Changed/Added Lines %
bam_masterdata/cli/cli.py 2 14 14.29%
bam_masterdata/cli/fill_masterdata.py 2 18 11.11%
bam_masterdata/cli/excel_to_entities.py 14 328 4.27%
Files with Coverage Reduction New Missed Lines %
bam_masterdata/cli/cli.py 1 38.89%
Totals Coverage Status
Change from base Build 12992047207: -1.9%
Covered Lines: 16400
Relevant Lines: 17187
💛 - Coveralls

JosePizarro3
JosePizarro3 reviewed Jan 27, 2025
View reviewed changes
bam_masterdata/cli/cli.py Outdated
(Optional) The path to the Masterdata Excel file.
""",
)
def fill_masterdata(url, path):
Copy link
Member

Choose a reason for hiding this comment

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

you have to change path to excel_file in this function

maybe click has another option for aliases if you prefer so

@JosePizarro3
Copy link
Member

Ok, I went locally through the branch and I got some points:

  • We need to change all print for logger messages. The level of the messages has to be clear.
  • I am having some errors when a cell.value is TRUE or FALSE. In my case, openpyxl is correctly reading it as a boolean, while I think for you, @carlosmada22, it is a string. My openpyxl version is 3.1.5.
  • Some of the logic is repeated between the ifs, e.g., the case when a cell.value is TRUE or FALSE. This can go in its own function to avoid repetition.

@carlosmada22 @JosePizarro3
Fixed permId getter
8b97316
@carlosmada22 @JosePizarro3
Fixed ruff formatting
7a46044
@carlosmada22 @JosePizarro3
Fetched object_type headers from excel to dict
8efd491
@carlosmada22 @JosePizarro3
Corrected linting
ab2e326
@carlosmada22 @JosePizarro3
First conversor from one entity excel file to dict
c7ff8b5
@carlosmada22 @JosePizarro3
Minor changes
ba8101e
@carlosmada22 @JosePizarro3
Starting to treat each entiy as individual blocks
402da0e
@carlosmada22 @JosePizarro3
Entities to dict by blocks finished
0b10c7f
@carlosmada22 @JosePizarro3
Added excel_to_entities to cli using fill_masterdata
55d2a63
@carlosmada22 @JosePizarro3
Finished and working parser using CLI from masterdata excel file to p…
c1778bb 
…ython model entities
@carlosmada22 @JosePizarro3
Added sorting for entities dictionary by inheritance
dd914dc
@carlosmada22 @JosePizarro3
added checker to properties
5424a4c
@carlosmada22 @JosePizarro3
added checker to vocabulary terms
61e8cee
@carlosmada22 @JosePizarro3
added checker to entity headers
f4acaad
@carlosmada22 @JosePizarro3
Checks and bug fixing, all working properly and conversion done
f842530
@carlosmada22 @JosePizarro3
Added missing docstrings, comments, typings and review notes
69ea0fa
@carlosmada22 @JosePizarro3
Fied mypy checks for dict typing
84c6ebb
@carlosmada22 @JosePizarro3
Fixed mypy checks and reviewed bugs and comments
4d5900f
@carlosmada22 @JosePizarro3
Fixed mypy checks
830362b
@JosePizarro3
Fixing mypy and other extras (#61)
75c4e1f 
Added str_to_bool function

Fix mypy in excel_to_entities

Added tmp/ to gitignore
@carlosmada22 carlosmada22 merged commit cf9d54e into main Jan 28, 2025
8 checks passed
@carlosmada22 carlosmada22 deleted the excel_to_entities branch January 28, 2025 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JosePizarro3 JosePizarro3 JosePizarro3 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Define overwrite and update for excel import-export
3 participants
@carlosmada22 @coveralls @JosePizarro3