Fork me on GitHub
#lumo
<
2017-08-22
>
richiardiandrea15:08:14

I have the following, but I see :

(defn ^:export -main [& args]
  (.log js/console cljs.core/*command-line-args*))
but *command-line-args* is always null, am I doing anything wrong? Lumo is 1.7.0

richiardiandrea15:08:32

I read that command-line-args is bound only in repl is that true? I know I can use process.argv but I was wondering if I need to open an issue

dominicm15:08:53

process.argv is kinda meh in lumo because it includes lumo itself. (iirc)

richiardiandrea15:08:30

yeah I have just tried and you are exactly right

anmonteiro15:08:20

Argv will also include "node" in Node.js

anmonteiro15:08:31

There might be a but wrt command line args

richiardiandrea15:08:35

process.argv includes two items that are not the arguments by default

anmonteiro15:08:04

There's a bug right here

richiardiandrea15:08:06

but @dominicm is right, if I use it in lumo it will include lumo args as well

richiardiandrea15:08:52

I was grepping for command-line-args and could not understand why I could not find it 😄

anmonteiro15:08:53

We switched yo cljs.core/*command-line-args*

richiardiandrea15:08:02

yes, I will push a PR, thanks!

richiardiandrea15:08:37

I was also looking at the other PR, I will need to ask you something as well, in a bit 😉

richiardiandrea16:08:08

@anmonteiro when you have time, I have an error here a bit difficult to understand for me https://github.com/anmonteiro/lumo/pull/238

anmonteiro17:08:41

@richiardiandrea so the reason you’re getting an error is because you never initialize the CLJS engine

richiardiandrea17:08:42

oh yeah, I was trying the suggestion of the comments and it still did not work, it is probably the init issue then

anmonteiro17:08:44

@richiardiandrea that’s also not the place where you wanna add the code

anmonteiro17:08:51

because we check for repl below

richiardiandrea17:08:34

yep good we are discussing this, I was thinking about it...I guess this feature is valid also for non-repl usage

richiardiandrea17:08:44

actually especially for non-repl usage

anmonteiro17:08:01

@richiardiandrea this is only for non-REPL usage

richiardiandrea17:08:39

yep, so we don't want to put it in if (repl)...where do you suggest we put it...I put it there because it is alternative to -m

anmonteiro17:08:05

I didn’t suggest to put it in that if statement

anmonteiro17:08:16

I suggested to place it in the else condition of that if statement

richiardiandrea17:08:33

but maybe it should be also checking if (!repl)

anmonteiro17:08:45

we’ll be there if !mainNsName && !repl

anmonteiro17:08:39

@richiardiandrea does *command-line-args* work with your fix?

richiardiandrea17:08:55

tnx for the suggestion 😉

anmonteiro17:08:35

I think we can now add tests for that too

anmonteiro17:08:39

since we run tests in Lumo

richiardiandrea17:08:03

successfully passing (cli-options (clj->js *command-line-args*)) and posix-getopts is happy

richiardiandrea17:08:31

the only thing I need is to set optind to 0

anmonteiro17:08:33

^ this namespace is the entry point for Lumo tests

anmonteiro17:08:02

so we can add Lumo-specific unit tests and require them from there

anmonteiro17:08:33

or even add unit tests in the other files and run them if test-util/lumo-env?

richiardiandrea17:08:16

oh I contributed to that already ah ah 😄 will do my best

anmonteiro17:08:40

@richiardiandrea for your current use case it would be just a matter of adding cli opts here https://github.com/anmonteiro/lumo/blob/master/circle.yml#L54

anmonteiro17:08:52

or even abstract that away in a common script that you call from the other ci scripts

richiardiandrea17:08:47

I guess that would be more of an integration test (that should be done), then I was thinking of setting *command-line-args* in a fixture or something for unit testing

richiardiandrea18:08:40

ah! the other patch does not work either...I am investigating why:

~/git/lumo/build/lumo scripts/lumo/build.cljs 
             ⬆
WARNING: No such namespace: lumo.repl, could not locate lumo/repl.cljs, lumo/repl.cljc, or JavaScript source providing "lumo.repl" at line 1 
             ⬆
WARNING: Use of undeclared Var lumo.repl/run-main-cli-fn at line 1 
command line arguments are required.
brb

richiardiandrea18:08:42

k solved and pushed the changes

richiardiandrea18:08:07

about the cli args, I think the only useful test is the one you are hinting at, an integration test from command line invocation...I don't see the point in testing something else

anmonteiro18:08:20

we can punt on that for now too

anmonteiro18:08:39

I’ve been meaning to set up integration tests but haven’t found time to do it yet

anmonteiro18:08:19

@richiardiandrea if that’s something you’d like to contribute, I’m open to ideas

richiardiandrea18:08:08

definitely something interesting to think about

anmonteiro18:08:19

there are some linting issues in your PR that’s why CI is failing

anmonteiro18:08:26

@richiardiandrea just running yarn prettier should fix them

anmonteiro18:08:34

or npm run prettier

richiardiandrea18:08:39

lol why is it adding a comma before a closing paren?

richiardiandrea18:08:48

I thought it was a typo and removed it

anmonteiro18:08:25

in ES6 trailing commas before closing parens are valid

anmonteiro18:08:51

(which is how you tell that TC-39 is really focusing on important issues)

dominicm19:08:35

I mean, that it wasn't confuses me

dominicm19:08:04

Death by semicolons I guess

dominicm19:08:13

lisp is so freeing

michaelwfogleman19:08:33

@anmonteiro @richiardiandrea I just started trying some basic scripts with Lumo - am I understanding that command-line-args is currently not functioning, but PR 237 will fix that?

michaelwfogleman19:08:14

OK, thought I was doing something dumb 😛

richiardiandrea19:08:58

if tests are going through and Antonio has time for that, a 1.7.1 might be good. Or master.

anmonteiro19:08:13

@michaelwfogleman once I merge that PR you can brew install --HEAD lumo if you’re on a mac

anmonteiro19:08:51

this is a bad enough bug that warrants cutting a 1.7.1 release

anmonteiro20:08:06

@richiardiandrea just wrap that entire deftest in an if or something

richiardiandrea20:08:36

done that, waiting for the results

anmonteiro20:08:46

does that work locally?

richiardiandrea21:08:21

awesome all green partywombat

richiardiandrea22:08:10

Added things to both Travis and AppVeyor confs...did not know you had three...that's good, this should now complete fine

anmonteiro22:08:08

ah cool, missed that