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

Clang format #9

Merged
merged 8 commits into from
Apr 5, 2022
Merged

Clang format #9

merged 8 commits into from
Apr 5, 2022

Conversation

Vikort
Copy link
Contributor

@Vikort Vikort commented Mar 12, 2022

Moved pr

@Vikort Vikort requested a review from KovalM March 18, 2022 17:25
@Vikort Vikort force-pushed the clang-format branch 2 times, most recently from 06e0fe0 to c14de01 Compare March 23, 2022 09:09
@KovalM KovalM requested a review from FallenChromium March 23, 2022 11:28
@KovalM KovalM added this to the 0.6.1 milestone Mar 23, 2022
Copy link
Member

@KovalM KovalM left a comment

Choose a reason for hiding this comment

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

Add check formatting to CI, script for only check formatting. Look to source PR, this changes are presented there

Copy link
Collaborator

@FallenChromium FallenChromium left a comment

Choose a reason for hiding this comment

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

format-code.py seems a bit redundant. What do you think about a CMake integration? Something like this

@Vikort Vikort requested review from KovalM and FallenChromium March 25, 2022 13:26
@Vikort Vikort force-pushed the clang-format branch 3 times, most recently from f66563e to 48ce715 Compare April 1, 2022 09:19
Copy link
Member

@KovalM KovalM left a comment

Choose a reason for hiding this comment

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

  1. Add information about using code check to docs
  2. Add setup pre-push hooks for code check. Add to docs info about install hooks

@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@0640ff8). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main       #9   +/-   ##
=======================================
  Coverage        ?   57.77%           
=======================================
  Files           ?       30           
  Lines           ?     3706           
  Branches        ?        0           
=======================================
  Hits            ?     2141           
  Misses          ?     1565           
  Partials        ?        0           
Flag Coverage Δ
unittests 57.77% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0640ff8...54d16be. Read the comment docs.

@Vikort Vikort requested a review from KovalM April 4, 2022 09:40
Copy link
Collaborator

@FallenChromium FallenChromium left a comment

Choose a reason for hiding this comment

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

I see a major problem with this approach:

cd build
cmake ..
make

First, it changes the current directory which is undesirable. Second, it relies on make rather than CMake only, so if we're going to switch the build system someday (say cmake -G Ninja), our docs will be obsolete. I have a proposal: let's make this build command a standard:

cmake -B build
cmake --build build

It's shorter, only relies on CMake and it doesn't do the cd. If we'll adopt this approach in this PR, I'll make another one to redact it in the remaining places. I am not asking about this out of purely aesthetics reasons, look at what happened on my machine, for example:
image
I already have a build folder, but that shouldn't be a problem! If the script just used build as a target directory through cmake, rather than trying to create it manually, that would've passed.
I've also sent some minor notes about the documentation, but other than the problem stated above and docs changes, it looks good to me! The formatting rules themselves are a subject of discussion, personally I don't like how it looks, but it's very subjective so I'm not going to bring this up unless someone else wants to elaborate on the topic.

docs/build/linux-build.md Outdated Show resolved Hide resolved
docs/build/linux-build.md Outdated Show resolved Hide resolved
scripts/ci/check-formatting.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@FallenChromium FallenChromium left a comment

Choose a reason for hiding this comment

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

Looks good to me! We could get rid of other pushd/popd or cd/cd .. logic too while we're at it, but it's unnecessary to do it in this PR.

@Vikort Vikort merged commit 1f4147b into ostis-ai:main Apr 5, 2022
@Vikort Vikort deleted the clang-format branch April 5, 2022 10:20
NikitaZotov pushed a commit to NikitaZotov/sc-machine that referenced this pull request Apr 4, 2023
chapter_top_ontologies added
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.

5 participants