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

Extend the to_excel() method and add to CLI #331

Merged
merged 17 commits into from
Mar 12, 2024

Conversation

danielhuppmann
Copy link
Member

This PR adds extends the method DataStructureDefinition.to_excel() to include all dimensions and add an overview-sheet with project name, a timestamp and urls/commits/timestamps of external repositories.

Copy link
Contributor

@phackstock phackstock left a comment

Choose a reason for hiding this comment

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

Looks very nice, thanks a lot for this.
A few comments and questions in line.

nomenclature/definition.py Outdated Show resolved Hide resolved
tests/test_cli.py Outdated Show resolved Hide resolved
nomenclature/definition.py Outdated Show resolved Hide resolved
nomenclature/definition.py Show resolved Hide resolved
nomenclature/definition.py Outdated Show resolved Hide resolved
nomenclature/definition.py Outdated Show resolved Hide resolved
nomenclature/cli.py Outdated Show resolved Hide resolved
nomenclature/cli.py Outdated Show resolved Hide resolved
nomenclature/cli.py Outdated Show resolved Hide resolved
Copy link
Contributor

@phackstock phackstock left a comment

Choose a reason for hiding this comment

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

One small remark, after that good to merge from my side.

dsd = DataStructureDefinition(TEST_DATA_DIR / "general-config" / "definitions")
dsd.to_excel(file)

obs = pd.ExcelFile(file)
Copy link
Contributor

Choose a reason for hiding this comment

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

Although not a critical as in the pyam issue IAMconsortium/pyam#817, I'd also use a context manager here to make sure the file is properly closed at the end of the test:

Suggested change
obs = pd.ExcelFile(file)
with pd.ExcelFile(file) as obs:

Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented, thanks for the suggestion

Copy link
Contributor

@phackstock phackstock left a comment

Choose a reason for hiding this comment

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

Good to be merged

@danielhuppmann danielhuppmann merged commit 4fbfc61 into IAMconsortium:main Mar 12, 2024
8 checks passed
@danielhuppmann danielhuppmann deleted the export/to_excel branch March 12, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants