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

refactor: js bindings #209

Merged
merged 10 commits into from
Oct 28, 2022
Merged

refactor: js bindings #209

merged 10 commits into from
Oct 28, 2022

Conversation

hyj1991
Copy link
Member

@hyj1991 hyj1991 commented Oct 27, 2022

将所有的 JS Binding 收敛到 src/jsapi 目录,方便后续统一增加管理。

此 PR 不涉及到逻辑改动,依靠 JS 单测保证逻辑兼容。

Reference:#27

@codecov
Copy link

codecov bot commented Oct 27, 2022

Codecov Report

Merging #209 (1eb95d1) into master (c2d224c) will not change coverage.
The diff coverage is n/a.

❗ Current head 1eb95d1 differs from pull request most recent head 7d62a3a. Consider uploading reports for the commit 7d62a3a to get more accurate results

@@            Coverage Diff            @@
##            master      #209   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines          296       296           
=========================================
  Hits           296       296           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -1,4 +1,4 @@
#include "configure.h"
#include "include/export_configure.h"
Copy link
Member

Choose a reason for hiding this comment

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

这个 include 目录不是公开 api 吧,这样按照惯例直接和 cc 文件放一起就可以了吧?

Copy link
Member Author

Choose a reason for hiding this comment

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

放一起会不会文件太多了,内部的非公开 include 目录有啥约定么,还是惯例就是全部平铺开的

Copy link
Member Author

Choose a reason for hiding this comment

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

我先改回来放到 jsapi 目录下平铺开

Copy link
Member Author

Choose a reason for hiding this comment

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

现在把 jsapi 的声明统一放到 src/jsapi/jsapi.h 了,去掉了 include 目录以及拆分的每一个 jsapi 的头文件,这样子似乎清晰一些,毕竟内容并不多

Copy link
Member

Choose a reason for hiding this comment

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

铺平的吧,按照模块分文件夹就好了。最好还是声明和实现用同一个名字的文件,比如 a.h 里声明的在 a.cc 里实现,这样不需要借助 ide、搜索就能比较方便定位代码。

Copy link
Member Author

Choose a reason for hiding this comment

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

改回来了,现在是铺平,然后头文件和实现名称保持一致

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

👍

@hyj1991 hyj1991 merged commit 289770f into X-Profiler:master Oct 28, 2022
@hyj1991 hyj1991 mentioned this pull request Dec 11, 2022
hyj1991 added a commit that referenced this pull request Dec 11, 2022
Commits:

  - [bfe59c1] feat: support http profiling (#215)
  - [85065b8] refactor: config (#214)
  - [ea2e7f8] fix: avoid copying constant strings (#212)
  - [5b157fe] fix: remove unused dump action field (#211)
  - [840df2a] refactor: consolidate dump actions (#210)
  - [289770f] refactor: js bindings (#209)

PR-URL: #216
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