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

Minor fixes for #338 #339

Merged
merged 5 commits into from
Jan 6, 2015
Merged

Minor fixes for #338 #339

merged 5 commits into from
Jan 6, 2015

Conversation

eugeneia
Copy link
Member

Minor fixes alongside the documentation PR #338 :

Includes one possibly controversial change: Make "App modules" not return the app classes so you can do:

  App = require("path").App

I did this because most apps did it this way and I wanted all apps to behave the same. It's arguable if this is right.

  App = require("path").App

instead of

  App = require("path")
apps.vpn. Make these convert string MAC and IP addresses.
argument and fall back to `_parse.ulp'.
attempting to append to multi-buffer packet payload.
Instead of an "incomplete" flag, return iovec index of the
remaining packet buffer or `nil' if datagram is "complete".
@eugeneia
Copy link
Member Author

eugeneia commented Jan 5, 2015

@lukego This should be merged alongside the documentation PR (already merged).

@lukego
Copy link
Member

lukego commented Jan 5, 2015

Looks like a valuable activity to me. I'm really happy to see some editing to gradually bring our varied and experimental coding styles closer together.

@alexandergall what do you think? you wrote most of the affected code.

@alexandergall
Copy link
Contributor

It's certainly good to make the usage consistent across all apps. My only objection would be that a lot of the require code looks a bit awkward because most modules provide a single app that tends to have the same name as the module, e.g.

local baz = require("foo.bar.baz").baz

A nice one is

local RateLimiter = require("apps.rate_limiter.rate_limiter").RateLimiter

But I can live with that :)

@lukego
Copy link
Member

lukego commented Jan 6, 2015

Yeah. It would sure be nice to do away with those require lines entirely. Perhaps we could cook up a simple convention for giving the app name as a string and for config.lua to infer an app name.

One step at a time.. :-)

lukego added a commit that referenced this pull request Jan 6, 2015
@lukego lukego merged commit 841079b into snabbco:master Jan 6, 2015
@eugeneia
Copy link
Member Author

eugeneia commented Jan 6, 2015

Just so it shows up on the record: The precedent is set by basic_apps, intel10g and pcap which are modules each providing multiple apps. So this is not only consistency but also the most common denominator.

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

Successfully merging this pull request may close these issues.

3 participants