Fixing bug in changing key and desc column sizes

[raphaelcohn]

This change ensures column width is actually respected.

For an example of usage, see https://github.com/libertine-linux/cvetool/blob/master/modules/cvetool/commandLineArgumentsParser.lua which sets column widths based on COLUMNS, terminal presence and terminal width (and uses the values for standard our or standard error, depending on whether help is to be displayed on stdout because -of -h or because of an error on stderr) and https://github.com/libertine-linux/cvetool/blob/master/modules/cvetool/init.lua for a real program.

[amireh]

Now that I've tried this, I can't see the problem. set_colsz is effective for me, it seems to work on the examples.

Following the code, look at https://github.com/amireh/lua_cliargs/blob/master/src/cliargs/core.lua#L42 where you can see that the printer is passed a function that returns the current state of the parser which will include the (maybe-updated) reference to the colsz table. The printer uses that at printing time, so it doesn't really cache any references.

I'm starting to think that you're seeing a different problem, but I just can't see how this fix fixes it. Can you please provide steps to reproduce? I'd been testing with the examples in the repository and they seem to accept custom colsz just fine.

Thanks!

[raphaelcohn]

It's to do with the timing of creation. I'd strongly encourage you to use a much clearer object model rather than one that makes extensive use of closures and scope capture. I'd also not have the library code printing to a file handle as well as generating a message in the same layer. Separation of concerns and all that. In fact, make the printer a loosely coupled concern. You can close this pull. I've forked your code and it suits my needs for now. Thanks for a good parser.

On 14 Jan 2017 11:39 a.m., "Ahmad Amireh" notifications@github.com wrote:

Now that I've tried this, I can't see the problem. set_colsz is effective for me, it seems to work on the examples.

Following the code, look at https://github.com/amireh/lua_ cliargs/blob/master/src/cliargs/core.lua#L42 where you can see that the printer is passed a function that returns the current state of the parser which will include the (maybe-updated) reference to the colsz table. The printer uses that at printing time https://github.com/amireh/lua_cliargs/blob/master/src/cliargs/printer.lua#L85, so it doesn't really cache any references.

I'm starting to think that you're seeing a different problem, but I just can't see how this fix fixes it. Can you please provide steps to reproduce? I'd been testing with the examples in the repository and they seem to accept custom colsz just fine.

Thanks!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/amireh/lua_cliargs/pull/56#issuecomment-272618837, or mute the thread https://github.com/notifications/unsubscribe-auth/ABPx2UhevoLBnfxMxFE6LglNxkvxMXXXks5rSLPmgaJpZM4LhuYs .

[amireh]

If something (like a bug fix) is useful to others, it'd be nice to merge it upstream rather than keeping it in a fork.

The library has been working the way it is, and if something proves to be dysfunctional as we come across different use-cases, I am happy to revisit (whether that be a design issue or an implementation detail.) However, I don't feel like your response is helpful which is unfortunate. I could not reproduce the issue outside of your sample, and the two pieces of code seem to be functionally equivalent (that's what I'm curious about; how you're coming across this issue when others aren't.)

Anyway, thanks for the request and good luck. Feel free to close the issue if you don't intend to follow up.

[alerque]

I also cannot replicate this. Yes a new table is being created, but the variable being set doesn't change scope and whether you swap out the table values or the whole table seems to make no material difference.

I'd be happy to re-consider if this were presented with a test case showing the failing situation first. Like Ahmad noted this seems like you might have been chasing a different bug.

Again I'll gladly re-open if any evidence to the contrary is brought. Thanks for the report & attempted fix.