Skip to content
This repository has been archived by the owner on Oct 2, 2020. It is now read-only.

fixes #5 #6

Closed
wants to merge 8 commits into from
Closed

fixes #5 #6

wants to merge 8 commits into from

Conversation

heapwolf
Copy link

@heapwolf heapwolf commented Dec 1, 2014

a quick fix for ensuring that the release downloaded is the latest.

@max-mapper
Copy link
Contributor

@hij1nx this is really cool!

my only concern is that getting the latest version every time is essentially doing atom-shell: '*' in dependencies, which makes things non-deterministic by default.

what do you think about making this opt-in, e.g. if you do ATOM_VERSION=latest npm install atom-shell it gets the latest version, otherwise it gets the version specified in the source code?

@heapwolf
Copy link
Author

heapwolf commented Dec 3, 2014

That seems like a valid concern from one perspective. I was under the impression though that this was more of a dev tool than a dependency, so is allowing * really a problem (especially considering how unstable atom shell is and how each release is basically a bunch of bug fixes)?

@max-mapper
Copy link
Contributor

IMO even if it's a devDependency it should still get versioned. It avoids a problem where the code worked when you wrote it and then 6 months later someone tries to get it to run and it breaks

@heapwolf
Copy link
Author

heapwolf commented Dec 3, 2014

Well, it's hard to ague that because I agree. What If it defaults to a version but uses a flag to pull latest?

@max-mapper
Copy link
Contributor

+1 that seems like a good solution

@jprichardson
Copy link

Yeah, love this proposal. @mafintosh thoughts?

@mafintosh
Copy link
Collaborator

I'm all for this. Sorry for being slow. @hij1nx could you resolve the conflict so I can merge this? :)

var version = '0.19.5'
var name = 'atom-shell-v'+version+'-'+platform+'-'+arch+'.zip'
var url = 'https://github.com/atom/atom-shell/releases/download/v'+version+'/atom-shell-v'+version+'-'+platform+'-'+arch+'.zip'
var arch = (platform == 'win32') ? 'ia32' : os.arch()

Choose a reason for hiding this comment

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

var arch = platform === 'win32' ? 'ia32' : os.arch()

Copy link
Author

Choose a reason for hiding this comment

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

platform is always a string, so === is pointless. parens for readability.

@sindresorhus
Copy link

Would also be nice if you could give this pull request a descriptive title.

@max-mapper max-mapper closed this Jun 19, 2015
@heapwolf
Copy link
Author

lol, just realized this is a dead PR anyway. ffs.

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

Successfully merging this pull request may close these issues.

5 participants