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

Formatting: JsThemis native extension #412

Merged
merged 9 commits into from Mar 7, 2019

Conversation

forelocked-beobachter
Copy link
Contributor

Add native extension of JsThemis to make fmt target group.

This requires quite a bit of effort because clang-tidy insists on having access to all source code involved in compilation, including all those NodeJS headers and its modules.

First of all, we have to add some hardcoded paths to NodeJS headers. I have not found a proper way to get these paths so we just hardcode them and hope that this is fine.

Then we need to have <nan.h> header file for which we have to install the nan module. We do a little hack to achieve that, pretending to 'format' the installation path for Node modules.


I have to say I'm not a fan of these hacks that we have to perform in order to get clang-tidy work on our source code...

ilammy and others added 4 commits March 5, 2019 16:17
Add native extension of JsThemis to "make fmt" target group.

This requires quite a bit of effort because clang-tidy insists on having
access to *all* source code involved in complation, including all those
NodeJS headers and its modules.

First of all, we have to add some hardcoded paths to NodeJS headers.
I have not found a proper way to get these paths so we just hardcode
them and hope that this is fine.

Then we need to have <nan.h> header file for which we have to install
the nan module. We do a little hack to achieve that, pretending to
'format' the installation path for Node modules.
This will make automated formatting a bit more nice.
Install npm and nodejs as we need them for checking JsThemis native
source code. At least for now.
Print out a warning when formatting files if NodeJS is not installed and
we are not able to reformat JsThemis files.
The current build system for libthemis-src is quite messed up and
requires peculiar symlinking to work. Add a symlink for the new file
included from the main Makefile.
@vixentael
Copy link
Contributor

What's the reasoning behind re-ordering headers like in this commit?
2639ee8

Copy link
Contributor

@vixentael vixentael left a comment

Choose a reason for hiding this comment

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

overall i like it

@ilammy
Copy link
Collaborator

ilammy commented Mar 6, 2019

@vixentael,

What's the reasoning behind re-ordering headers like in this commit?
2639ee8

Consistency with other code that (now) uses include order suggested by Google Coding style:

  • main header
  • system headers
  • library headers
  • private headers

Reordering itself does not change anything semantically, but I have optimized the headers a bit (such as removed <vector> include where we already include it via the main header).

@ilammy ilammy merged commit 4d11421 into cossacklabs:master Mar 7, 2019
@ilammy ilammy deleted the format-jsthemis branch March 7, 2019 14:56
@ilammy ilammy added W-JsThemis 🍭 Wrapper: JsThemis for Node.js, JavaScript API, npm packages and removed E-Node.js labels Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
W-JsThemis 🍭 Wrapper: JsThemis for Node.js, JavaScript API, npm packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants