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

MR27: Remove use of installed_packages for threejs #28086

Closed
sagetrac-gitlab-bot mannequin opened this issue Jun 30, 2019 · 20 comments
Closed

MR27: Remove use of installed_packages for threejs #28086

sagetrac-gitlab-bot mannequin opened this issue Jun 30, 2019 · 20 comments

Comments

@sagetrac-gitlab-bot
Copy link
Mannequin

sagetrac-gitlab-bot mannequin commented Jun 30, 2019

Isuru Fernando (@isuruf) opened a merge request at https://gitlab.com/sagemath/sage/merge_requests/27:


Depends on #28007

CC: @saraedum @paulmasson @kiwifb @antonio-rojas @timokau

Component: graphics

Keywords: threejs

Author: Isuru Fernando

Branch/Commit: c643b25

Reviewer: Paul Masson

Issue created by migration from https://trac.sagemath.org/ticket/28086

@sagetrac-gitlab-bot sagetrac-gitlab-bot mannequin added this to the sage-8.9 milestone Jun 30, 2019
@isuruf

This comment has been minimized.

@sagetrac-gitlab-bot
Copy link
Mannequin Author

sagetrac-gitlab-bot mannequin commented Jun 30, 2019

comment:2

New commits added to merge request. I updated the commit SHA-1. New commits:

1b17e52add missing import os

@sagetrac-gitlab-bot
Copy link
Mannequin Author

sagetrac-gitlab-bot mannequin commented Jun 30, 2019

Changed commit from 97fe175 to 1b17e52

@sagetrac-gitlab-bot
Copy link
Mannequin Author

sagetrac-gitlab-bot mannequin commented Jun 30, 2019

Changed commit from 1b17e52 to ad4e8a9

@sagetrac-gitlab-bot
Copy link
Mannequin Author

sagetrac-gitlab-bot mannequin commented Jun 30, 2019

comment:3

New commits added to merge request. I updated the commit SHA-1. New commits:

ad4e8a9Add r to version

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Jun 30, 2019

comment:4

While this will certainly work, what about a global variable THREEJS_VER evaluated just once?

@kiwifb
Copy link
Member

kiwifb commented Jun 30, 2019

comment:5

I am rather indifferent on how this is implemented. But even if create a variable THREEJS_VER in env.py it will be evaluated each time you call? Or are you thinking of hard-coding the version? I am not a big fan of hardcoding myself, is it really that detrimental?

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Jun 30, 2019

comment:6

Was thinking env.py only ran once, but if it runs ever time it is called then there will be no difference.

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Jun 30, 2019

Reviewer: Paul Masson

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Jun 30, 2019

Dependencies: #28007

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Jun 30, 2019

comment:7

Works for me

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Jun 30, 2019

Changed keywords from none to threejs

@fchapoton
Copy link
Contributor

comment:8

Patchbot reports several doctest failures, guys..

And patchbot plugins are not green either.

@kiwifb
Copy link
Member

kiwifb commented Jul 1, 2019

comment:9

That's because the dependency at #28007 is merged and closed but not in any release or beta yet. I am surprised it merges on the patchbots.

@fchapoton
Copy link
Contributor

comment:11

ok then, but still the plugins "pyflakes" and "pycodestyle" are not green.

@sagetrac-gitlab-bot
Copy link
Mannequin Author

sagetrac-gitlab-bot mannequin commented Jul 2, 2019

comment:12

New commits added to merge request. I updated the commit SHA-1. New commits:

c643b25Fix formatting error

@sagetrac-gitlab-bot
Copy link
Mannequin Author

sagetrac-gitlab-bot mannequin commented Jul 2, 2019

Changed commit from ad4e8a9 to c643b25

@isuruf
Copy link
Member

isuruf commented Jul 2, 2019

comment:13

pyflakes error is for a line I didn't change. I've fixed pycodestyle error.

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Jul 10, 2019

comment:14

Any good reason not to set this back to positive review?

@vbraun
Copy link
Member

vbraun commented Jul 14, 2019

Changed branch from u/galois/mrs/27/threejs to c643b25

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

No branches or pull requests

4 participants