Skip to content

Change formatting of ESP8266 source files to project standards. #600

New issue

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

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

Already on GitHub? # to your account

Closed
nkolban opened this issue Sep 29, 2015 · 13 comments
Closed

Change formatting of ESP8266 source files to project standards. #600

nkolban opened this issue Sep 29, 2015 · 13 comments
Assignees
Labels
ESP8266 This is only a problem on ESP8266 devices low priority

Comments

@nkolban
Copy link
Contributor

nkolban commented Sep 29, 2015

The new ESP8266 source files do not adhere to the project coding formatting standards. Specifically, tabs are used with a tabstop of 4. This work item looks to rework the source files to adhere to standards ... specifically, no tabs and an indent of 2.

@nkolban nkolban added low priority ESP8266 This is only a problem on ESP8266 devices labels Sep 29, 2015
@nkolban
Copy link
Contributor Author

nkolban commented Oct 4, 2015

@tve Has made substantial progress in #611.

@tve
Copy link
Contributor

tve commented Oct 4, 2015

I'm happy to do a pass at this, but I've held off until gordon merges all my changes because the formatting changes really obscure anything else going on. It has to happen as a separate PR...

@gfwilliams
Copy link
Member

Thanks - good call wrt holding off on this IMO.

Thanks for trying to bring stuff in line.

@nkolban for single-line comments, please can you skip writing \brief? Don't worry about the existing stuff but IMO it makes it more difficult if you're reading the code inline (without Doxygen). I'm also not sure it makes a difference? Doxygen definitely has an option for 'treat up to first . as a \brief' that I can turn on if you want?

@nkolban
Copy link
Contributor Author

nkolban commented Oct 5, 2015

@gfwilliams Perfect ... not a problem. I'll remove the \brief .... we probably need to examine the coding standards to address proper Doxygen generation ... for example ... ive also been using \return to create a description of what the function logically returns. We probably want to see if there is a non tagging way to achieve that too.

@gfwilliams
Copy link
Member

Hi guys,

I just came across this:

/*JSON{
  "type" : "constructor",
  "class" : "SPI",
  "name" : "SPI",
  "generate" : "jswrap_spi_constructor"
}
Create a software SPI port. This has limited functionality (no baud rate), but it can work on any pins.

Use `SPI.setup` to configure this port.
 */

/**
 * \brief Create a software based SPI interface.
 * \return A JS object that will hold the state of the SPI.
 */
JsVar *jswrap_spi_constructor() {
  return jsvNewWithFlags(JSV_OBJECT);
}

Please can you not add the Doxygen documentation on top of what's there? There's already documentation there, which is specially designed to get parsed and output as part of the existing Espruino reference pages (while also being used to build the symbol table).

Just rewording the same text further down really doesn't help (IMO) - and it moves the important, machine parsed bit away from the implementation itself. I know Doxygen probably doesn't parse it as-is, but there's got to be a better way of getting Doxygen docs than by duplicating every one of probably 1000 different document blocks.

Maybe we can get Doxygen to run a script beforehand such that it tweaks the comment blocks, so the Doxygen'd functions get documented properly and also show the JSON - that could be properly handy.

@gfwilliams
Copy link
Member

Looks like we were both writing comments at the same time - sorry about that :)

I do have some python that pulls out the comments, so maybe we could get it to re-write the header files in a Doxygen-friendly format, before it gets run?

@nkolban
Copy link
Contributor Author

nkolban commented Oct 5, 2015

Is there a Wiki page or project markdown file that we can use to record the standards we wish to follow?

@gfwilliams
Copy link
Member

Sure, at the moment there's the existing code style stuff under: https://github.com/espruino/Espruino/blob/master/CONTRIBUTING.md#coding-style

Maybe we should just expand that out into a whole file though?

Definitely the more stuff we can do automatically the better (for instance I recently ran through with Eclipse's auto-formatter on the codebase to try and keep things looking ok).

We ought to be explicit about using Unix newlines too, but I just don't 'get' Git's newline handling. I think at the moment there's probably quite a mix of file types.

@nkolban
Copy link
Contributor Author

nkolban commented Oct 5, 2015

I had also been making documentation changes in commits when I came across changes. I believe that this is frowned upon (and I can easily see why). I will change my behavior and now will only make comment changes in code that is "locally" being changed. For example, if I am changing function XYZ in file ABC.c then I will ONLY make commenting changes for XYZ and directly related areas.

For documentation changes in the code that are "housekeeping", I will create separate commits that will be included in their own Pull Requests ... I think that is what is desired ... if not ... please help me to be a better citizen.

@nkolban
Copy link
Contributor Author

nkolban commented Oct 6, 2015

Removed spurious function from jshardware.c called jshFlashContainsCode that was not referenced anywhere within the project. This seems to have been a hangover that came from who-knows-where.

@nkolban
Copy link
Contributor Author

nkolban commented Oct 6, 2015

Removed all the additions of \brief that were added for Doxygen.

@gfwilliams
Copy link
Member

That's awesome - thanks for all your help with this!

Yes, definitely separate commits for doc changes are better (unless they're related to the change). The main reasoning if if you're tracing a bug back to a certain commit (git bisect is amazing - it's worth googling) then it helps to have just the code that changed in that commit so you can see clearly what happened.

@nkolban nkolban self-assigned this Oct 6, 2015
@nkolban
Copy link
Contributor Author

nkolban commented Oct 7, 2015

All tabs replaced by two spaces in ESP8266 supplied code. Fixed in #623.

@nkolban nkolban closed this as completed Oct 7, 2015
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
ESP8266 This is only a problem on ESP8266 devices low priority
Projects
None yet
Development

No branches or pull requests

3 participants