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

Add support for similar waste types with different API IDs #1267

Merged
merged 2 commits into from
Dec 7, 2024

Conversation

EyeDevelop
Copy link
Contributor

Proposed Changes

This PR adds a fall-through method to the WasteType enum. The Twente Milieu API might return a waste type that is almost the same as one already defined in the enum.

Hengelo now has pick-up points for packaging and paper for high-density living. For the packaging, they created a new waste type. However, it is still packaging. This change now maps the 'odd' waste type returned from the API to the packages value in the enum.

Please let me know what you think!

Related Issues

#1265

When attempting to run the `updateContentCommand` while building the development container, it returned an error with git as cause. The log stated that the directory was not marked safe, so now it does that before the `updateContentCommand`.
@frenck frenck added the enhancement Enhancement of the code, not introducing new features. label Dec 6, 2024
Copy link
Owner

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Hi there @EyeDevelop 👋

CI is failing, could you please take a look?

../Frenck

@frenck frenck marked this pull request as draft December 6, 2024 22:16
@EyeDevelop EyeDevelop force-pushed the feature/new-waste-type-56 branch from c81a352 to 96591d5 Compare December 7, 2024 12:27
@EyeDevelop
Copy link
Contributor Author

Hi @frenck, I (should) have resolved all the issues with CI. Can you take a look?

I like the idea of having strict coding guidelines enforced with these typing rules and test coverage. However, if I could make a suggestion it would be to loosen them in some cases. For the _missing function I added to the IntEnum, mypy made a note that value should be of type object. However, in practice this can never be anything other than int unless the _missing_ function is directly called. You can see in the test I added that I have to call the method directly to comply with the 100% coverage. It is my opinion that these are not really useful additions, but I leave this up to you.

@EyeDevelop EyeDevelop marked this pull request as ready for review December 7, 2024 12:33
@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (7c783e6) to head (95481e9).
Report is 362 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1267   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines           95       100    +5     
  Branches        15        12    -3     
=========================================
+ Hits            95       100    +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@frenck frenck left a comment

Choose a reason for hiding this comment

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

However, if I could make a suggestion it would be to loosen them in some cases.

Sorry... I strongly disagree very much. Don't know what else to say.

Left a few review comments and the CI is also failing (which is odd? As ruff is run in pre-commit... not sure how you manage to make a commit that passes it).

../Frenck

.devcontainer/devcontainer.json Show resolved Hide resolved
src/twentemilieu/twentemilieu.py Outdated Show resolved Hide resolved
tests/test_twentemilieu.py Outdated Show resolved Hide resolved
@EyeDevelop EyeDevelop force-pushed the feature/new-waste-type-56 branch from 96591d5 to 95481e9 Compare December 7, 2024 13:10
@EyeDevelop
Copy link
Contributor Author

Processed your review comments. Let me know if I should drop the first commit with the changes to the dev container file.

Copy link
Owner

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Thanks, @EyeDevelop 👍

../Frenck

@frenck frenck merged commit 90c0013 into frenck:main Dec 7, 2024
13 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Enhancement of the code, not introducing new features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants