sympy_gamma
sympy_gamma copied to clipboard
sympy
Solves & Fixes Issue #35 | Now constant multiplies the integral
So, this resolves and fixes Issue #35 . Now, constant multiplies the integral.
Same can be seen in the screenshot attached below-:
@asmeurer Please have a look at this PR.
Thanks. :smile:
I guess this fix is taken from https://github.com/sympy/sympy_gamma/pull/60. How difficult would it be to take that branch and finish it up in a new pull request?
@asmeurer actually I thought that, since that PR didn't get pushed, hence there might be some problem in that (because I thought that PR might have been for some testing purpose). Hence, I pushed this new PR. So, should I delete this PR? Or will this get merged?
I'm unclear why the PR wasn't pushed. If @lidavidm is around maybe he can shed some light on it. My guess is that it was just never done. There is only one unchecked TODO mentioned (and I'm unsure how to test that without deploying).
I would take his branch, merge it with master, then create a new pull request from that, and go ahead and test it to see if everything appears to be working.
Sorry, yeah, I think I never got around to it. As I remember, I just never got around to testing the ALLOWED_HOSTS setting, which can't really be tested without actually deploying it, unfortunately. (It might just be easier to set a wildcard, though I don't recall the security implications of that).
@lidavidm Ohh! So, Since your PR had some issue, should we go ahead with this PR for merging? Or what? @asmeurer
Thanks! :smile:
@ashutoshsaboo I believe the PR did work, so you could try merging it into your pull request. If the ALLOWED_HOSTS stuff is too difficult to work through, we could just not update the Django version. (We don't really use Django features, and I updated just to make sure we didn't fall too far behind and weren't vulnerable to anything security-wise. However, we already do much more insecure things than use an old Django version.)
Let's try to get @lidavidm's branch merged with master and merge it in. If it breaks the search box on sympy.org we'll revert it.
Yes, but I guess, we must update the Django version definitely. Because, as Django updates, it might deprecate several of it's previous features. So, for security reasons, I do feel that, updating the Django version is pretty essential.
As, such at this stage itself, the django version, used in SymPy Gamma is 1.3 , on the other hand the latest django version is I guess 1.9.4 which is way newer to our version.
Also, many people generally have the latest version of Django installed, since no where in the wiki it's mentioned that the version 1.3 of Django is required. I also had the latest version of Django installed, and it led to Error #70 . I couldn't figure it out for a lot of while, and then only I came to know about this.
Hence I think we must definitely consider updating the version of Django, to avoid such issues.
Thanks! :smile:
@asmeurer @lidavidm Your views on this^?
In addition to that, Updating SymPy to the latest version is also necessary.
I have pulled the latest version of SymPy. I am just checking in case if there are any issues with the latest version of SymPy on SymPy Gamma, and then I'll push the PR for that soon.
I agree we should do those things. I'm not sure if it's better to merge @lidavidm's branch, update Django, and update SymPy in three separate pull requests or to do them all at once. Likely doing them separately is better. Keep in mind that if there is a failure after a deploy we will have to revert to the previous deploy, meaning if all the changes are pushed in at once and one of them causes an issue, we will have to revert all of them until we can get it fixed.
Yes, exactly. I agree with you @asmeurer Sir!
That's what, it's better to do it in different PR's, because in case, even if you have to revert, only a small code will change. Otherwise, in case if a large code changes, it'll be hard to figure out, then.
Also, this has passed all the Travis CI Tests as well after #73 got merged.
I have tested for this PR on my local machine as well, and it did pass all of the tests. @asmeurer So, I guess this doesn't seem to have any issue. I'm also working on the SymPy version as well, and i'll see only if there's no problem, then I'll push the PR.
So, do we go ahead with this PR for merging - @asmeurer Sir? @lidavidm
Thanks! :smile: