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

Jump to bottom

fix(excel2resources, excel2properties): cover all cases (DEV-1040) #201

Merged
merged 28 commits into from Jun 23, 2022
Merged

fix(excel2resources, excel2properties): cover all cases (DEV-1040) #201

merged 28 commits into from Jun 23, 2022

Conversation

jnussbaum
Copy link
Collaborator

resolves DEV-1040

@jnussbaum
fix bugs
78f2a3f 
make more robust
@jnussbaum jnussbaum self-assigned this Jun 15, 2022
@jnussbaum jnussbaum changed the title fix(excel2resources, excel2properties): make more robust, correct schemas fix(excel2resources, excel2properties): cover all cases (DEV-1040) Jun 15, 2022
@jnussbaum jnussbaum changed the title fix(excel2resources, excel2properties): cover all cases (DEV-1040) fix(excel2resources excel2properties): cover all cases (DEV-1040) Jun 15, 2022
@jnussbaum jnussbaum marked this pull request as ready for review June 16, 2022 14:40
@jnussbaum jnussbaum changed the title fix(excel2resources excel2properties): cover all cases (DEV-1040) fix(excel2resources, excel2properties): cover all cases (DEV-1040) Jun 16, 2022
jnussbaum
jnussbaum commented Jun 16, 2022
View reviewed changes
MANIFEST.in Show resolved Hide resolved
jnussbaum
jnussbaum commented Jun 16, 2022
View reviewed changes
docs/dsp-tools-excel.md Outdated Show resolved Hide resolved
jnussbaum
jnussbaum commented Jun 16, 2022
View reviewed changes
docs/dsp-tools-excel.md Outdated Show resolved Hide resolved
BalduinLandolt
BalduinLandolt approved these changes Jun 16, 2022
View reviewed changes
Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a comment

Choose a reason for hiding this comment

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

Looks good overall. I'm concerned the regex might miss some edge cases... and stripping all strings in the beginning would spare you from doing so all over the place.

.github/workflows/check-pr-title.yml Show resolved Hide resolved
knora/dsplib/utils/excel_to_json_properties.py Outdated Show resolved Hide resolved
knora/dsplib/utils/excel_to_json_properties.py Outdated Show resolved Hide resolved
knora/dsplib/utils/excel_to_json_resources.py Outdated Show resolved Hide resolved
test/unittests/test_excel_to_properties.py Outdated Show resolved Hide resolved
irinaschubert
irinaschubert suggested changes Jun 17, 2022
View reviewed changes
Copy link

@irinaschubert irinaschubert left a comment

Choose a reason for hiding this comment

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

Could you add one or several more unit tests which test the special characters and numbers cases you discussed? Then we can be sure that it works as expected.

.github/workflows/check-pr-title.yml Show resolved Hide resolved
knora/dsplib/schemas/lists-only.json Show resolved Hide resolved
knora/dsplib/utils/excel_to_json_properties.py Outdated Show resolved Hide resolved
knora/dsplib/utils/excel_to_json_resources.py Outdated Show resolved Hide resolved
test/unittests/test_excel_to_properties.py Outdated Show resolved Hide resolved
BalduinLandolt
BalduinLandolt approved these changes Jun 21, 2022
View reviewed changes
Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a comment

Choose a reason for hiding this comment

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

I didn't look closely at the tests. Everything else seems good to me - all my comments are of cosmetic nature.

knora/dsplib/utils/excel_to_json_resources.py Outdated Show resolved Hide resolved
knora/dsplib/utils/excel_to_json_properties.py Outdated Show resolved Hide resolved
knora/dsplib/utils/excel_to_json_properties.py Outdated Show resolved Hide resolved
knora/dsplib/utils/excel_to_json_properties.py Outdated Show resolved Hide resolved
knora/dsplib/utils/excel_to_json_properties.py Show resolved Hide resolved
knora/dsplib/utils/excel_to_json_resources.py Show resolved Hide resolved
irinaschubert
irinaschubert approved these changes Jun 23, 2022
View reviewed changes
test/e2e/test_tools.py Outdated Show resolved Hide resolved
@jnussbaum
- restore test_excel_to_json_resources() and test_excel_to_json_prop…
8b77cce 
…erties()

 - delete test_validate_ontology()
test_tools.py is now a complete collection of all dsp-tools subcommands
@sonarcloud
Copy link

sonarcloud bot commented Jun 23, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@jnussbaum jnussbaum merged commit 4c6ed19 into main Jun 23, 2022
@jnussbaum jnussbaum deleted the wip/dev-1040-improve-excel2resources-excel2properties branch June 23, 2022 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BalduinLandolt BalduinLandolt BalduinLandolt approved these changes

@irinaschubert irinaschubert irinaschubert approved these changes

Assignees

@jnussbaum jnussbaum

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jnussbaum @irinaschubert @BalduinLandolt