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.

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

Add checking of lua sources to travis build #2423

Merged
merged 8 commits into from
Jul 16, 2018

Conversation

HHHartmann
Copy link
Member

@HHHartmann HHHartmann commented Jul 9, 2018

  • 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.

There are several *.lua files that are not compilable.
This PR adds checking lua files in travis builds.
Travis build will fail if there is a lua file in lua_modules or lua_samples that cannot be compiled.

@HHHartmann
Copy link
Member Author

So found two more build errors in Lua code
Ready for integration now. (from my POV only of corse)

@marcelstoer marcelstoer merged commit 2d2c189 into nodemcu:dev Jul 16, 2018
@marcelstoer marcelstoer added this to the next-release milestone Jul 16, 2018
@TerryE
Copy link
Collaborator

TerryE commented Jul 16, 2018

@HHHartmann Gregor normally what I do when I am working on a patch is to rebaseline against (the current) dev before pushing to my GitHub fork and issuing the PR. In my case because my patches are often big and take time, dev has usually moved on since I created my branch so this is even more needed, but we recommend this practice. Maybe for your next commit, eh? 😊

@HHHartmann
Copy link
Member Author

@TerryE Terry I do not quite understand what you ask me to do (or even not to do). Shouldn't I have merged the remote-tracking branch? I am not yet very familiar with git so I would be more than happy to be enlightened a bit more.
But most of all let me thank you for the great LFS patch. It is great to have telnet and ftp running and still have 40k of RAM left.

@TerryE
Copy link
Collaborator

TerryE commented Jul 16, 2018

@HHHartmann Gregor, it is far more important that you feel comfortable contributing which is why Marcel and I were happy to merge your PR with multiple commits. The rebasing stuff is covered here in the online Git book, but to be honest I get confused with the advanced features of git at times! Sometime I'll get a round to a simple 1-2-3 guide.

@HHHartmann
Copy link
Member Author

@TerryE Terry, thanks for the link. I will read it and find out how to do it. But still I wonder what I should have done? Should it have been only one commit? Puzzled and curious.

@HHHartmann HHHartmann deleted the travis_check_lua branch July 16, 2018 20:33
@TerryE
Copy link
Collaborator

TerryE commented Jul 16, 2018

But still I wonder what I should have done

For a change of this size and scope I would had one or two commits, and if two then I would done the *.lua changes as one commit.

@HHHartmann
Copy link
Member Author

Ah ok.
I had to do separate commits here because I could see no way to test beforehand as the travis build only gets triggered there. But I suppose I could have squashed them afterwards? Or reduce them by first thinking, then committing.
Thanks four your guidance anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants