ftpserver mutated without adapting documentation - breaking change

[HHHartmann]

Expected behavior

code works as documented

Actual behavior

code fails with errormessage:

node.flashindex('ftpserver')().createServer("test","12345",true)
Lua error: 	...itHub\nodemcu-firmware\lua_modules\ftp\ftpserver.lua:70: attempt to index a string value
stack traceback:
	...itHub\nodemcu-firmware\lua_modules\ftp\ftpserver.lua:70: in function 'createServer'
	stdin:1: in main chunk
	[C]: ?
	[C]: ?

Test code

Working code:

node.flashindex('ftpserver')():createServer("test","12345",true)

Note the ':' instead of '.' before createServer

Documentation says:

httpserver.createServer()

NodeMCU startup banner

This is on current dev and master branch

NodeMCU 3.0.0.0
branch: pr_3158
commit: fd8ef76
release: 3.0-master_20200610 +11
release DTS:
SSL: false
build type: float
LFS: 0x40000 bytes total capacity
modules: adc,bit,color_utils,crypto,dht,encoder,enduser_setup,file,gpio,http,i2c,l3g4200d,mdns,net,node,ow,pipe,pixbuf,pwm,rtcfifo,rtcmem,rtctime,sjson,sntp,spi,struct,tmr,uart,wifi,wifi_monitor,ws2812,ws2812_effects
build 2020-06-26 19:39 powered by Lua 5.1.4 on SDK 3.0.1-dev(fce080e)

[TerryE]

My bad. Yes, this is a minor breaking change that needs documenting. The old ftp server could only be run out of LFS which is an impediment for beginners. I've made some tweaks so the file can be split into dynamically loaded separate LC files for top level methods and this runs fine out of RAM or the #3117 fast loader, but this required you to pass the ftp table in as an arg hence the need for the object notation.

I will fix the documentation.

[HHHartmann]

The bad thing is that this is on Master already.
Do we have a policy for fixing issues on Master?

Btw how can it be split?

[TerryE]

We are talking about a few line change to the MD file. If @marcelstoer is OK with this, I'll just patch the master documentation.

The split stuff is discussed in #3117 and my gists. Sorry about this small gremlin, but the whole Lua 5.1 to 5.3 program is complex and a lot of tidying is needed and we should realistically expect the odd bump.

Thanks for picking this up.

[marcelstoer]

If @marcelstoer is OK with this, I'll just patch the master documentation.

On master or on release? And what about RTD?

If you commit this to master you'll probably have to merge it back to dev. Otherwise we'll have conflict next time we drop to master.

[TerryE]

master. I know that we've created release but we can't make the formal cut until ReadTheDocs has got its support for redirection in place.

I'll just make the same change to Dev.

[HHHartmann]

Should we also mention this in the Breaking Change section of the release?

[nwf]

Since we're hot-patching master, we should do criss-crossing merges to dev when we do the next release.

[TerryE]

You're the expert here, not me 🤣

[marcelstoer]

I'll just make the same change to Dev.

That's what will cause conflicts.

we should do criss-crossing merges to dev when we do the next release.

👍 except that I propose this happens immediately after the master patch

• no unpleasant surprises while cutting a release
• memory of what the change is all about is still fresh
• no need to manually weave the same change into dev

[nwf]

Seems OK by me. Commit to master, merge the new master into dev, push new master and dev to github. Everybody's repos should be happy with that sequence. I think I agree with @marcelstoer that we should make this the policy for every commit on master/release that isn't just a re-tagging of dev.

[marcelstoer]

2 months have passed and we're approaching another master drop. IMO the only thing still relevant at this point is to fix it before the next master drop.

@HHHartmann just to be sure, only https://github.com/nodemcu/nodemcu-firmware/blob/dev/docs/lua-modules/ftpserver.md is affected?

[HHHartmann]

Yes

[marcelstoer]

Ok, I was confused since you mentioned the HTTP server

Documentation says:
httpserver.createServer()

[TerryE]

Gregor is correct. It is my bad. I changed the FTP slightly so it can be loaded into non-LFS configs. I'll do a quick tidy and push the fix PR. Sorry about this.

[HHHartmann]

Ok, I was confused since you mentioned the HTTP server

Documentation says:
httpserver.createServer()

Oh. Didn't see that one. But yes it is the ftpserver only