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

lang: funcs: New simple funcs #647

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

S-ign
Copy link

@S-ign S-ign commented Mar 4, 2021

Tips:

  • please read the style guide before submitting your patch:
    docs/style-guide.md

  • commit message titles must be in the form:

topic: Capitalized message with no trailing period

or:

topic, topic2: Capitalized message with no trailing period

  • golang code must be formatted according to the standard, please run:
make gofmt		# formats the entire project correctly

or format a single golang file correctly:

gofmt -w yourcode.go
  • please rebase your patch against current git master:
git checkout master
git pull origin master
git checkout your-feature
git rebase master
git push your-remote your-feature
hub pull-request	# or submit with the github web ui
  • after a patch review, please ping @purpleidea so we know to re-review:
# make changes based on reviews...
git add -p		# add new changes
git commit --amend	# combine with existing commit
git push your-remote your-feature -f
# now ping @purpleidea in the github PR since it doesn't notify us automatically

Thanks for contributing to mgmt and welcome to the team!

@purpleidea
Copy link
Owner

@S-ign Okay-- to continue this learning experience, here are some lessons:

  1. When submitting a patch (to most projects in general) it's considered OK for the maintainer to ignore it completely unless it passes all the tests. So you should check your patch passes, or ask for help from the maintainer in understanding the failures.

  2. You had a previous open PR ... learning and tests #638 ... You should have force-pushed your updated changes to that instead of opening a new one. No big deal, but you know for next time.

  3. When updating a PR (for example, when you push or force-push a new commit) make sure you ping with a little message after the tests pass like "tests passed, ready for review" or something, since GH doesn't always notify the maintainers of new commits to a PR.

HTH

@S-ign S-ign changed the title updated lang: funcs: New simple funcs Mar 5, 2021
@S-ign S-ign force-pushed the feat/tests branch 4 times, most recently from 9dbe137 to b5cb657 Compare March 9, 2021 00:58
@purpleidea
Copy link
Owner

@S-ign Okay, we had a fun git hackthon today, and all looks good test wise =D
It looks like you've added some extra functions and things in here. The bad news is that we autogenerate a bunch of these already. If you look in lang/funcs/core/generated_funcs.go this will get built when you run make. And stuff that's in there (mostly stuff from the stdlib) doesn't need to be written again fortunately.
Good news, I think this was all a good learning experience though, and you've learned the important things to tackle the next bit:

So what's next?

  1. You've figured out the boilerplate, and so lets morph this code into something we can merge!

  2. How about a function is_macaddress that does what you think it should do =D I think it should live in the mcl net package.

  3. Leave this PR open for now, but try working on the above function and sending it as a new PR. Ping when it passes the tests (write a few please) and is ready and I'll review.

Side note: One thing you've probably learned now too is that sometimes when writing code for a new project, it's always good to check with the maintainers about the rough design of whatever you're building before you code it, in case there's a gotcha that you didn't know about. For the Count stuff, well, that was autogenerated. For the Ipport stuff, I need to think about if we want to add that, because as-is it's just $addr + ":" + $port (both are strings) where as we count morph it into a str+int version if that's valuable. Let's discuss that later...

Make sense? LMK =D

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.

2 participants