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

Correct several doxygen tags (\file,...) #696

Merged
merged 2 commits into from Jul 28, 2014
Merged

Conversation

remyleone
Copy link
Contributor

Cleaning doxygen warning such as

  • Correct several tags such as \file to make their argument point to the right file.
  • rename description to brief

@cmorty
Copy link
Contributor

cmorty commented Jun 2, 2014

👍

@darconeous
Copy link
Member

I'd prefer the whitespace changes be removed or factored out into a separate commit.

@g-oikonomou
Copy link
Contributor

👍 for the changes here, but I'm with @darconeous on this one. Please either tell your editor to stop auto-applying whitespace fixes or include them as a different commit. In reality this pull is +/- 60 lines

I won't merge this as-is, cause we'll start setting a bad precedent.

Thanks

@mcr
Copy link
Contributor

mcr commented Jun 4, 2014

------- Blind-Carbon-Copy

From: Michael Richardson mcr@sandelman.ca
To: Contiki developer mailing list contiki-developers@lists.sourceforge.net
Subject: git and whitespace errors.
In-Reply-To: contiki-os/contiki/pull/696/c45063364@github.com
References: contiki-os/contiki/pull/696@github.com contiki-os/contiki/pull/696/c45063364@github.com
X-Mailer: MH-E 8.2; nmh 1.3-dev; GNU Emacs 23.4.1
X-Face: $\n1pF)h^}$H>Hk{L"x@)JS7<%Az}5RyS@k9X%29-lHB$Ti.V>2bi.~ehC0;<'$9xN5Ub# z!G,pnR&p7Fz@^UXIn156S8.~^@mj*mMsD7=QFeq%AL4m<nPbLgmtKK-5dC@#:k
Date: Wed, 04 Jun 2014 09:01:07 -0400
Message-ID: 10975.1401886867@sandelman.ca
Sender: mcr@sandelman.ca

(After some pain on the tcpdump github, and a desire to include more of the
community in some issues that get brought up in issues, and also to have some
things in the mail archive, I've often replied to both issue and list.
Turns out that bad things happen if you leave the github reply-* in, so the
issue gets a BCC)

George Oikonomou notifications@github.com wrote:
> 👍 for the changes here, but I'm with @darconeous on this one. Please
> either tell your editor to stop auto-applying whitespace fixes or
> include them as a different commit. In reality this pull is +/- 60 lines
> I won't merge this as-is, cause we'll start setting a bad precedent.

I want to encourage everyone to run editors that strip trailing whitespace,
and I have some emacs lisp that I use for this. The problem is that any
line you've touched (if your editor bashes the current line only), or any
file you've visited, such as to turn on debugging, gets whitespace errors.

Here is what I do.
Work as normal. Try to isolate your work into smallest pieces you can with
git add -p, and commit very often (every hour or two). commit even if it
doesn't apply, but note that, because you'll remove those steps with commit

  • --amend, or during the rebase. Don't push to github.

Make a note of all files that you've touched (I use git whatchanged, and some
editing in a file, but I'll bet there is a better way to make that list).
Make a new branch without your changes:
git checkout -b whitespace origin/master
pull all the files you've changed into your editor and save them without
trailing whitespace (or use some shell script to do that).
You can verify that you haven't made any non-whitespace changes with:
git diff -B -w
it should result in an empty output.
git commit -m'whitespace changes'

Then, merge and/or rebase your changes branch on top. Resolve any conflicts,
which are likely just whitespace (some tools like kdiff3 can do this for you,
but I'm not an expert at which ones). If you merged, you probably want to
git rebase -i origin/master
again to put your code into a single chain rather than having a useless merge
point.
Now, you can push.

I've eben trying to figure out if there is a github way to create a separate
pull request for the whitespace changes difference, vs the later changes on
that branch (without creating two branches), but I haven't figured out if
there is a simple way to do that.

It would be nice if github could be told to reject and/or mark files that
have whitespace errors.


] Never tell me the odds! | ipv6 mesh networks [
] Michael Richardson, Sandelman Software Works | network architect [
] mcr@sandelman.ca http://www.sandelman.ca/ | ruby on rails [

------- End of Blind-Carbon-Copy

@remyleone
Copy link
Contributor Author

Done :) git add -p is really useful

@@ -2032,7 +2032,7 @@ CCIF extern uip_lladdr_t uip_lladdr;
(((a)->u8[12]) == 0xFF))

/**
* \briefput in b the solicited node address corresponding to address a
* \brief in b the solicited node address corresponding to address a
Copy link
Member

Choose a reason for hiding this comment

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

My previous comment about this seems to have disappeared. The word "put" was removed here, which made the sentence incomplete. If you can fix this, I think that the PR will be ready to merge.

@g-oikonomou
Copy link
Contributor

@sieben Rémy please address Nicolas' comment above when you get a moment and we'll clicky this one in.

Many thanks

@remyleone
Copy link
Contributor Author

OK I will do it as soon as I can.

2014-07-27 23:58 GMT+02:00 George Oikonomou notifications@github.com:

@sieben https://github.com/sieben Rémy please address Nicolas' comment
above when you get a moment and we'll clicky this one in.

Many thanks


Reply to this email directly or view it on GitHub
#696 (comment).

@remyleone
Copy link
Contributor Author

Done. ;)

@nvt
Copy link
Member

nvt commented Jul 28, 2014

👍

nvt pushed a commit that referenced this pull request Jul 28, 2014
Correct several doxygen tags (\file,...)
@nvt nvt merged commit 582bfcb into contiki-os:master Jul 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants