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

#79 Add 'COOLING' state to AirConditioningState #80

Merged

Conversation

smiitm
Copy link
Contributor

@smiitm smiitm commented Oct 5, 2024

This PR adds the missing 'COOLING' state to the AirConditioningState enum in response to issue #79. The issue reported a ValueError due to the missing 'COOLING' state, and I've updated the enum to include this state.

This is my first open source contribution, and I'm excited to be part of the community. Please feel free to provide any feedback or guidance if there's anything I can improve. Thanks.

@WebSpider
Copy link
Contributor

Thanks for your contribution! 🙏
Please have a look at the linter results and correct your PR as needed.

@@ -26,7 +26,7 @@ class AirConditioningState(StrEnum):
OFF = "OFF"
Copy link
Contributor

Choose a reason for hiding this comment

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

I also like when items are sorted alphabetically, it is easier to find the value and catch duplicates (may be not valid concern for such short enum but just to be consistent).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! It seems practical, and I will definitely keep it in mind. I’ve made the change while fixing the other issue.

@fursov
Copy link
Contributor

fursov commented Oct 6, 2024

Would be also nice to have a test case for this value. Please, have a look on tests/test_rest_api.py file for a function load_air_conditioning(). You can add your car's response to a fixture under folder tests/fixtures and then add the reference to the load_air_conditioning()

@smiitm
Copy link
Contributor Author

smiitm commented Oct 6, 2024

Thanks for your contribution! 🙏 Please have a look at the linter results and correct your PR as needed.

hey, i've fixed the linting issue.

@smiitm
Copy link
Contributor Author

smiitm commented Oct 6, 2024

Would be also nice to have a test case for this value. Please, have a look on tests/test_rest_api.py file for a function load_air_conditioning(). You can add your car's response to a fixture under folder tests/fixtures and then add the reference to the load_air_conditioning()

I will look into it. I wanted to confirm where the corresponding JSON file should be added. Should it go under enyaq/, superb/, or other/?

@fursov
Copy link
Contributor

fursov commented Oct 6, 2024

Would be also nice to have a test case for this value. Please, have a look on tests/test_rest_api.py file for a function load_air_conditioning(). You can add your car's response to a fixture under folder tests/fixtures and then add the reference to the load_air_conditioning()

I will look into it. I wanted to confirm where the corresponding JSON file should be added. Should it go under enyaq/, superb/, or other/?

Depends on the car model for which you have your response. E.g. I have superb, so I add my jsons to superb folder. If you do not know the car model, put to other.

Copy link
Contributor

@WebSpider WebSpider left a comment

Choose a reason for hiding this comment

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

Lgtm

@WebSpider
Copy link
Contributor

Thanks for your contribution 👍

@WebSpider WebSpider merged commit 8ebade8 into skodaconnect:main Oct 6, 2024
2 checks passed
@smiitm smiitm deleted the add-cooling-to-airconditioningstate branch October 6, 2024 15:53
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.

3 participants