-
Notifications
You must be signed in to change notification settings - Fork 1
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
Main #358
Main #358
Conversation
Bumps [github/issue-metrics](https://github.com/github/issue-metrics) from 2 to 3. - [Release notes](https://github.com/github/issue-metrics/releases) - [Commits](github/issue-metrics@v2...v3) --- updated-dependencies: - dependency-name: github/issue-metrics dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
…thub/issue-metrics-3 chore(deps): bump github/issue-metrics from 2 to 3
…ding existing and non-existent files, loading specific columns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @entelecheia - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
def save_dataframe( | ||
data: pd.DataFrame, | ||
data_file: str, | ||
data_dir: Optional[str] = None, | ||
columns: Optional[Sequence[str]] = None, | ||
index: bool = False, | ||
filetype: Optional[str] = "parquet", | ||
suffix: Optional[str] = None, | ||
verbose: bool = False, | ||
**kwargs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code_clarification): Consider documenting the **kwargs
parameter in the method docstring.
Explicit documentation of **kwargs
can help developers understand what additional parameters are expected or how they are used.
dataframe = DSLoad.load_dataframe(data_file, data_dir=data_dir) | ||
self.assertIsNotNone(dataframe) | ||
self.assertIsInstance(dataframe, pd.DataFrame) | ||
self.assertListEqual(list(dataframe.columns), list(data.columns)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Missing test cases for different file types and error conditions.
The test for save_dataframe
only covers a basic scenario. Consider adding tests for different file types (e.g., 'parquet', 'csv') as specified by the filetype
parameter. Additionally, include tests for error conditions such as invalid file paths or unsupported file types.
Merge pull request #358 from entelecheia/main
No description provided.