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

Solves & Fixes Issue #35 | Now constant multiplies the integral #74

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Solves & Fixes Issue #35 | Now constant multiplies the integral #74

wants to merge 1 commit into from

Conversation

ashutoshsaboo
Copy link
Contributor

So, this resolves and fixes Issue #35 . Now, constant multiplies the integral.

Same can be seen in the screenshot attached below-:

selection_011

@asmeurer Please have a look at this PR.

Thanks. 😄

@asmeurer
Copy link
Member

I guess this fix is taken from #60. How difficult would it be to take that branch and finish it up in a new pull request?

@ashutoshsaboo
Copy link
Contributor Author

@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?

@asmeurer
Copy link
Member

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.

@lidavidm
Copy link
Member

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).

@ashutoshsaboo
Copy link
Contributor Author

@lidavidm Ohh! So, Since your PR had some issue, should we go ahead with this PR for merging? Or what? @asmeurer

Thanks! 😄

@lidavidm
Copy link
Member

@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.)

@asmeurer
Copy link
Member

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.

@ashutoshsaboo
Copy link
Contributor Author

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! 😄

@ashutoshsaboo
Copy link
Contributor Author

@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.

@asmeurer
Copy link
Member

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.

@ashutoshsaboo
Copy link
Contributor Author

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.

@ashutoshsaboo
Copy link
Contributor Author

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.

@ashutoshsaboo
Copy link
Contributor Author

So, do we go ahead with this PR for merging - @asmeurer Sir? @lidavidm

Thanks! 😄

@ashutoshsaboo
Copy link
Contributor Author

Also, @lidavidm do we go ahead merging this? 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants