search

carrot / ship

⛔️ currently unmaintained ⛔️
Other
151 stars 14 forks source link

Argument output proposal #6

Closed kylemac closed 10 years ago

kylemac commented 10 years ago

IMO, the object we're returning from the argument parser is a little confusing. I've also run into issues where I'm expecting a relative path but receive the absolute process.cwd() for path. I also tend to think that path is a little bit of a confusing name in the context of the deployer.

I propose the following objects:

The new objects are cargo and folder. cargo is simply the path joined with the folder. I don't believe I've had a use-case for folder yet, and I'm not 100% pleased with it's naming - but it seems like down the line this operation would be necessary and it makes sense to marshall it now instead of doing a string difference of path and cargo later.

jescalan commented 10 years ago

This looks good, definitely agreed that the args parsing needed a little love. Is this piece tested and such? We should probably put this on travis.

Also, this might be of some interest: https://github.com/carrot/sprout/blob/master/lib/utils/accord.coffee

kylemac commented 10 years ago

no tests yet - wanted to propose it before I sunk too much time. I had a sneaking suspicion you had written something like accord. I'll dig into it when I get back into this refactor

jescalan commented 10 years ago

haha the complex arg parsing had started to drive me crazy. i don't think it would solve this problem exactly, just might be interesting. but yeah definitely agreed over here

hhsnopek commented 10 years ago

Are the listed objects going to be used? As I am going through I could make the changes. Edit: I just realized you guys have 7 branches >.<

hhsnopek commented 10 years ago

For all the targets in each deployer, the defined cargo in ship public gh-pages would be the target, minus ignored_files correct? If so each target(if defined) is the cargo defined from arg_parser, we'd want to pass this along to each deployer if I'm not mistaken

jescalan commented 10 years ago

Currently this is implemented if I'm not mistaken, the only change I made was cargo > payload

hhsnopek commented 10 years ago

For each deployer though, the @target == @cargo Seems it shouldn't be renamed in a different file?

hhsnopek commented 10 years ago

Within the gh-pages deployer for example the @config = target: null could be @config = @cargo if it was passed down

jescalan commented 10 years ago

@HHSnopek have you had a chance to pull this down and work with it at all? I think that might make it easier to point out specific spots that you are looking to improve on : )

hhsnopek commented 10 years ago

haha exactly what I've been doing, yet the all the branches confuse me! Could you merge working branches? I tend to start fixing something, remember there's a branch for it, switch and it works no changes necessary from me haha :smile:

jescalan commented 10 years ago

Yup, no problem. You can work off master here. I've closed out all the other confusing branches I can. @kylemac quick check on the status on https://github.com/carrot/ship/tree/conf - can we remove this branch or is it something you are still working on?

hhsnopek commented 10 years ago

Thank you very much @jenius! :grinning:

hhsnopek commented 10 years ago

@jenius, this wasn't merged?

jescalan commented 10 years ago

No, but similar changes were made in https://github.com/carrot/ship/commit/882322453033388ef86a2aafd52d451f1748d9b8