-
Notifications
You must be signed in to change notification settings - Fork 12
dhis2
Consider removing "patch" and "discover" for new dhis2 version
#51
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
Comments
great - one more estimate on this one too pls @hunterachieng |
Let's keep patch - we'll dump it in the Let's drop discover. And early, so that it doesn't confuse us with other work |
@josephjclark Since the merged PR had this fixed, should we close it? |
Ach @hunterachieng, thank you for flagging. I merged that PR too early so you haven't had a chance to resolve this comment, but please make sure you address it in your next PR (along with async await). Even so, I almost missed this because:
It's super important that when PRs are marked for review, the PR title and description are up to date and accurate, and all relevant issues area tagged in the PR. Before you mark the PR ready for review, it's really important that you:
I do this for every single PR I open. And yes I still make mistakes! But this is a really big omission from the PR description and we shouldn't really be in this position right now |
This is noted and I shall address all these in the next PR. |
Fixed in #1077 |
See these pull requests: OpenFn/language-dhis2#53 and OpenFn/language-dhis2#54
It's possible that neither of these operations are required any longer, but we should probably not make drastic changes until we consider the new shape of the dhis2 (with magic functions for visual building) and take into account dhis2 api changes.
The text was updated successfully, but these errors were encountered: