Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Makes lfs/_init.lua compatible with Lua 5.3 #3168

Merged
merged 1 commit into from
Sep 3, 2020
Merged

Makes lfs/_init.lua compatible with Lua 5.3 #3168

merged 1 commit into from
Sep 3, 2020

Conversation

vsky279
Copy link
Contributor

@vsky279 vsky279 commented Jun 18, 2020

Fixes #3156.

  • This PR is for the dev branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/*.

This commit makes lua_examples/lfs/_init.lua compatible with Lua 5.3.

nwf
nwf requested changes Jun 19, 2020
View reviewed changes
lua_examples/lfs/_init.lua Show resolved Hide resolved
lua_examples/lfs/_init.lua Outdated Show resolved Hide resolved
lua_examples/lfs/_init.lua Show resolved Hide resolved
@nwf nwf added the bugfix label Jun 19, 2020
@nwf nwf added this to the Next release milestone Jun 19, 2020
@vsky279
Copy link
Contributor Author

vsky279 commented Jun 19, 2020 •
edited
Loading

How do we solve there should probably be different luacheck Lua 5.1 and Lua 5.3?

@vsky279 vsky279 force-pushed the c_lfs_init branch 3 times, most recently from 89dd41d to 9decd7e Compare June 21, 2020 19:42
nwf
nwf approved these changes Jun 21, 2020
View reviewed changes
Copy link
Member

@nwf nwf left a comment

Choose a reason for hiding this comment

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

LGTM

@nwf
Copy link
Member

nwf commented Jun 24, 2020

This will need further revision in light of #3176. Blocked until that lands, I think.

@nwf nwf self-requested a review June 24, 2020 01:00
@nwf nwf added the blocked Waiting on another PR to land label Jun 24, 2020
@vsky279
Copy link
Contributor Author

vsky279 commented Jun 25, 2020

Merging now would mean to have the example compatible with Lua 5.3. Another rework should follow #3176 imho.

@nwf
Copy link
Member

nwf commented Jun 25, 2020

That seems like an OK plan to me. @TerryE?

@TerryE
Copy link
Collaborator

TerryE commented Jun 25, 2020

That seems like an OK plan to me. @TerryE?

There are +/-'s here. Anyone that needs _init.lua support for 5.3 can use this code, but I am trying to get the PR which addresses #3175, #3176 and #3180 out in the next few days, and merging this now means that I will have to do a rebase before I can create this PR which is going to add to this timeline.

BTW, it's my wife's birthday today, so I don;t want to be locked away in my study today. 😄

@nwf nwf mentioned this pull request Jun 28, 2020
Closed
@TerryE TerryE mentioned this pull request Jul 7, 2020
Closed
@nwf
Copy link
Member

nwf commented Aug 23, 2020

Now that #3176 has been closed, it's probably time to revisit this. Unblocking.

@nwf nwf removed the blocked Waiting on another PR to land label Aug 23, 2020
@vsky279 vsky279 force-pushed the c_lfs_init branch 3 times, most recently from 821984e to 78a2e9c Compare August 30, 2020 21:55
nwf
nwf reviewed Aug 30, 2020
View reviewed changes
tools/luacheck_config.lua Outdated Show resolved Hide resolved
tools/luacheck_config.lua
},
LFS = {
read_only = true,
fields = {
Copy link
Member

Choose a reason for hiding this comment

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

Not really a question for this PR, but since you're in the neighborhood: should we suggest to advanced developers how to add entries here? People using their own LFS might want their code to still be luacheck-clean?

Copy link
Member

Choose a reason for hiding this comment

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

@nwf I think it would be a good idea to make some kind of documentation on how to use luacheck on NodeMCU code. I've written luacheck_config_helper.lua script when I was adding luacheck support but I haven't documented it anywhere besides some comments in the script itself. Should this be added to GitHub Wiki, Extension Developer FAQ or somewhere else?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be a good idea to make some kind of documentation on how to use luacheck on NodeMCU code.

I am more of the view: what's wrong with RTFM? If anyone wants to read up on how to use luacehck then it comes with a lot of good documentation. May a reference to it in our FAQs.

@marcelstoer
Copy link
Member

Since this is approved, is it ready to be merged?

@nwf
Copy link
Member

nwf commented Sep 3, 2020

Sure, what's the worst that happens. ;)

@nwf nwf merged commit a0d2705 into nodemcu:dev Sep 3, 2020
@vsky279 vsky279 deleted the c_lfs_init branch September 6, 2020 15:10
@galjonsfigur galjonsfigur mentioned this pull request Sep 9, 2020
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nwf nwf nwf approved these changes

@marcelstoer marcelstoer marcelstoer left review comments

@TerryE TerryE TerryE left review comments

@galjonsfigur galjonsfigur galjonsfigur left review comments

Assignees
No one assigned
Labels
bugfix
Projects
None yet
Milestone
September 2020
Development

Successfully merging this pull request may close these issues.
5 participants
@vsky279 @nwf @TerryE @marcelstoer @galjonsfigur