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

Updates.py #154

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Updates.py #154

wants to merge 3 commits into from

Conversation

Khushi-Dua
Copy link

@Khushi-Dua Khushi-Dua commented Oct 27, 2024

Description

Here’s a concise analysis of the code:

  1. Hardcoded Path for Non-Existent File:

    • The test for a non-existent file uses a fixed path, which can lead to issues if the environment changes. Using a dynamic path (like tmp_path) ensures portability across platforms.
  2. Assertion Enhancements:

    • The test for an existing file asserts modification_time > 0, assuming a positive timestamp indicates a valid file. However, adding an isinstance check ensures the return type matches expectations, enhancing robustness.
  3. Error Handling Expectations:

    • The test_get_modification_time_non_existing_file expects -1 for non-existent files. This assumption should be confirmed by the AllFileMetadata class to ensure consistent handling of errors without throwing exceptions.
  4. Improved Descriptions:

    • Slightly refining docstrings would clarify the intent of each test, making the code easier to understand and maintain.

These adjustments will make the tests more reliable, readable, and adaptable to different environments.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • Documentation update
  • Tests update

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • New and existing unit tests pass locally with my changes
  • I have maintained a clean commit history by using the necessary Git commands
  • I have checked that my code does not cause any merge conflicts

changes:
1. Removed Duplicate import pytest
2.Enhanced assertion in test_get_modification_time_existing_file
3.Removed hardcoded path in test_get_modification_time_non_existing_file
4.Clarified modification_time check
5.Temporary file fixture improvement
Here’s a concise analysis of the code:

1. **Hardcoded Path for Non-Existent File**:
   - The test for a non-existent file uses a fixed path, which can lead to issues if the environment changes. Using a dynamic path (like `tmp_path`) ensures portability across platforms.

2. **Assertion Enhancements**:
   - The test for an existing file asserts `modification_time > 0`, assuming a positive timestamp indicates a valid file. However, adding an `isinstance` check ensures the return type matches expectations, enhancing robustness.

3. **Error Handling Expectations**:
   - The `test_get_modification_time_non_existing_file` expects `-1` for non-existent files. This assumption should be confirmed by the `AllFileMetadata` class to ensure consistent handling of errors without throwing exceptions.

4. **Improved Descriptions**:
   - Slightly refining docstrings would clarify the intent of each test, making the code easier to understand and maintain.

These adjustments will make the tests more reliable, readable, and adaptable to different environments.
@Kota-Karthik
Copy link
Owner

@Khushi-Dua
please strictly follow the PR template, only after that your PR will be reviewed!

@Wahid7852
Copy link
Collaborator

Waiting for a response @Khushi-Dua. If no response under 24 hours then this PR will be closed.

@Khushi-Dua
Copy link
Author

@Khushi-Dua please strictly follow the PR template, only after that your PR will be reviewed!

@Kota-Karthik what is PR template?

@Kota-Karthik
Copy link
Owner

@Khushi-Dua
A PR template is a MD file that have some structure and you follow it when you open a PR
You did use the PR template but didn't fill all information like issue number, screenshots, description etc.

Khushi-Dua added a commit to Khushi-Dua/twinTrim that referenced this pull request Oct 29, 2024
@techy4shri
Copy link
Collaborator

By following the PR Template, we mean that you fill the fields which shown in the first comment, like mention the issue number and all. In this PR only. And resolve the merge conflicts too.

@techy4shri
Copy link
Collaborator

@Khushi-Dua reply by tonight or I will be closing this PR. If you need more help, ask your doubts and questions on discord channel.

@techy4shri techy4shri added the changes requested changes requested in the pull request to make it eligible for merging. label Nov 1, 2024
@Khushi-Dua
Copy link
Author

Khushi-Dua commented Nov 1, 2024

I’ve made the changes to the PR as discussed. Everything is now updated and ready for your review.

Please let me know if there are any further adjustments needed

@techy4shri
Copy link
Collaborator

techy4shri commented Nov 1, 2024

@Khushi-Dua you need to resolve the conflicts in this PR and mention the issue no. this PR fixes, the issue you were assigned, not the PR.

@techy4shri
Copy link
Collaborator

@Kota-Karthik I see the branch used is main.....think we should ask her to rebase?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes requested changes requested in the pull request to make it eligible for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants