-
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
Feature/smhi class to match 0.2.0 #115
Conversation
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.
A simplification.
I am not in a position to fix the unit tests without support. |
I hate mutating an object's state. I'd rather a method called I'd also write the unit tests in a way that will be easier for us when updating the code in the future. That is, as little patching as possible and I've simplified it a bit in 57c3e05. I'd probably add a separate method called |
I still think you should refactor the |
…m/Ingenjorsarbete-For-Klimatet/ifk-smhi into feature/smhi-class-to-match-0.2.0
Sorry @mgcth, didn't realize until now that you had also commited here. I will look into your work and update. Agree we shouldn't mutate the object. |
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.
Great job, looks better!
At this point, I'd focus on docs and one or two tests of the whole SMHI object (call them integration, but without calling the API).
Thanks for the input. I will try to get an integration test going and sort out the remaining stuff to the extent that I can. |
@mgcth it seems your fix to use IntEnum changed the order of the list so that default is now "corrected-archive" again, was that deliberate? |
Yes. Do you prefer it the other way around? |
@mgcth it seems your fix to use IntEnum changed the order of the list so that default is now "corrected-archive" again, was that deliberate?
Not at all - I thought you wanted it the other way around after that discussion we had earlier, but I have absolutely no opinion on the matter. I will continue with this way of working, seems good to me. 👍 |
Ping @mgcth |
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.
Looks good. Just minor comments from the phone. Fix them and I'll approve tomorrow when I have a computer at hand.
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.
Great job! 🚀
I don't know if I managed to get the figure in the build properly @mgcth, could you have a look please? Other than that, I am quite happy. |
Naive update of parent class to new structure. To be updated later. Ambition here is simply to support 0.2.0 release.