Skip to content

Greater dir naming flexibility. - #245

Merged
jkarni merged 1 commit into
masterfrom
jkarni/feature/createdevfolder
Jun 26, 2013
Merged

Greater dir naming flexibility.#245
jkarni merged 1 commit into
masterfrom
jkarni/feature/createdevfolder

Conversation

@jkarni

@jkarni jkarni commented Jun 21, 2013

Copy link
Copy Markdown

Accepts any dirname for the edx-platform repo. Allows the script to be
called from any directory, not just $BASE/edx-platform.

   Accepts any dirname for the edx-platform repo. Allows the script to be
   called from any directory, not just $BASE/edx-platform.
@jkarni

jkarni commented Jun 21, 2013

Copy link
Copy Markdown
Author

@cpennington ?

@yarko

yarko commented Jun 22, 2013

Copy link
Copy Markdown
Contributor

@jkarni: yes; I like this much better (thanks).

Comment thread scripts/create-dev-env.sh

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DO NOT IMPLEMENT (discussion only):
Could head this off by following the idiom as for $PROJECT_HOME set PROJECT_NAME to 'edx-platform' if not set (e.g. after line 120).

It would not be a bad habit to get out of hardwiring constants like this anyway (.virtualenv comes to mind).
The downside (for a PROJECT_NAME) is that would be 21 more lines to change, for dubious benefit (there is clear purpose for PROJECT_HOME).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For rvm (and probably also virtualenv), I'm not sure it's worth not hardcoding the name. People can still make and switch to other ones, and I can't imagine that they have a -different- virtualenv that has something else entirely but is named edx-platform.

On the other hand, I do think it might be a good idea to have something as simple as PROJECT_NAME=edx-platform along with all the other constant defs, since we do use it in a couple of different places.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(continuing discussion only) - re: hardcoding: first: taking out literal values tends to immune your code from changes over time (e.g. .virtualenv was the default from virtualenvwrapper, but now is Envs).

The second (more significant) benefit: your setup code (at least snippets) will prove more portable, more easily adapted / modified to other sub-projects as they spring up for several reasons: [1] one (as opposed to many) thing to change (e.g. PROJECT_NAME), and [2] the entire idiom of get something from a variable encourages coding so as to "not make assumptions" [e.g. "what is the "name"?", "what are the parameters it needs?", etc.].

Of course, all that needs be weighed against obfuscation: clearer (simpler) is better. In the case of PROJECT_NAME, if I were starting from scratch, that would be a wash for me (could go either way); PROJECT_HOME is more concrete in motivation. Anyway, that's how I think about it. YMMV.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I don't think PROJECT_NAME is really worth it, but the (your) PROJECT_HOME change definitely was.

@Slater-Victoroff

Copy link
Copy Markdown
Contributor

Ah yes, great change. Assuming you've run this on a machine I say go ahead and merge it.

jkarni pushed a commit that referenced this pull request Jun 26, 2013
@jkarni
jkarni merged commit 38d7ddf into master Jun 26, 2013
@jkarni
jkarni deleted the jkarni/feature/createdevfolder branch June 26, 2013 15:19
chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014
Added style and better markup for the quotes on the jobs page
ataki pushed a commit to ataki/edx-platform that referenced this pull request Mar 30, 2015
…msSettings

Fixing settings for course_forums queue
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