Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix SQL in procedure drilldown report #2224

Merged
merged 1 commit into from
Apr 13, 2023
Merged

Conversation

gebilaoman
Copy link
Contributor

Here are two grammatical errors, the source of which was updated on February 16, 2023

@chrisknoll
Copy link
Collaborator

Thank you for finding this issue. I think we could accept this PR off of your master but I would like to caution you about working this way. Instead you should think about branching off master (even in your own fork) to fix changes. So, the workflow is:

  1. Identify the issue and make a issue out of (note the number of the issue here)
  2. In your fork, create a branch named issue-xxxx or issue-xxx-{short text about bug}
  3. Commit your changes into your branch, and create a PR that referenes your fork's branch with fix.

This may sound nit-picking, but it will help you out in the future if we are taking multiple PRs into the upstream master branch, your local master branch will not have those commits and I think it would get more complicated than if you just isolate your changes to a side branch.

@anton-abushkevich , I understand you guys are finalizing the release for 2.13. This is a bug that was introduced when fixing the casting problem based on numeric. I just malformed the query in the case of the procedure drilldown. Can this be merged in for 2.13 or did you want to get 2.13 done and fix this in a hotfix?

@chrisknoll chrisknoll changed the title Master Fix SQL in procedure drilldown report Mar 10, 2023
@chrisknoll chrisknoll merged commit a1a130a into OHDSI:master Apr 13, 2023
@gebilaoman
Copy link
Contributor Author

Thank you for finding this issue. I think we could accept this PR off of your master but I would like to caution you about working this way. Instead you should think about branching off master (even in your own fork) to fix changes. So, the workflow is:

  1. Identify the issue and make a issue out of (note the number of the issue here)
  2. In your fork, create a branch named issue-xxxx or issue-xxx-{short text about bug}
  3. Commit your changes into your branch, and create a PR that referenes your fork's branch with fix.

This may sound nit-picking, but it will help you out in the future if we are taking multiple PRs into the upstream master branch, your local master branch will not have those commits and I think it would get more complicated than if you just isolate your changes to a side branch.

@anton-abushkevich , I understand you guys are finalizing the release for 2.13. This is a bug that was introduced when fixing the casting problem based on numeric. I just malformed the query in the case of the procedure drilldown. Can this be merged in for 2.13 or did you want to get 2.13 done and fix this in a hotfix?

yeah, I think you are right,thank you

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants