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

build: use built-in formatter #1276

Closed

Conversation

ErtanTaner
Copy link

@ErtanTaner ErtanTaner commented Jul 23, 2023

Fixes #1274.

Changes

I changed outadated formatter with built-in one.

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@ErtanTaner ErtanTaner requested a review from a team July 23, 2023 12:27
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 23, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@Kielek
Copy link
Contributor

Kielek commented Jul 24, 2023

@BurakTaner,
please sign EasyCLA before we can proceed.
I suppose that it will detect some issues which we need to fix together with this merge.

@Kielek Kielek added the infra Infra work - CI/CD, code coverage, linters label Jul 24, 2023
@ErtanTaner
Copy link
Author

Hi @Kielek, I tried to sign EasyCLa with your email and name-surname but gave me error "Company not found". My first time with EasyCLA, can you give a little help?

@Kielek
Copy link
Contributor

Kielek commented Jul 26, 2023

@BurakTaner, you have two options

  1. Register as individual contributor
  2. Register as a part of the company were you working on. It depends on when you want to contribute to OTel repositories. In this case you have to find manager in your company to proceed.

@ErtanTaner ErtanTaner force-pushed the build/change-formatter branch from c5dd71c to 1ff0434 Compare July 26, 2023 09:34
@ErtanTaner
Copy link
Author

@Kielek I signed CLA and pushed last changes to make branch up to date with main. What should ı do next?

@@ -29,4 +29,4 @@ jobs:
run: dotnet tool install -g dotnet-format
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's part of the newer .NET SDK, then do we still need to install dotnet-format?

@@ -29,4 +29,4 @@ jobs:
run: dotnet tool install -g dotnet-format

- name: dotnet format
run: dotnet-format --folder --check
run: dotnet format .\opentelemetry-dotnet-contrib.sln --no-restore --verify-no-changes
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to specify the .sln file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you simplify this command by removing the parameter for providing the .sln file?

Copy link
Author

Choose a reason for hiding this comment

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

I tried to use withtout specify the sln but gives me error. I checked from documentation and they said "If you don't specify a a file, going to check for current directory to found a solution" but it's not in my local computer.

@utpilla
Copy link
Contributor

utpilla commented Jul 27, 2023

Since this PR does not change any .cs file or .editorconfig, the dotnet format check is not being triggered. We need to ensure that this check works correctly before we merge this PR. You might have to update a .cs file and then check the dotnet-format run, before undoing the change to the .cs file.

@Kielek
Copy link
Contributor

Kielek commented Jul 27, 2023

@BurakTaner, I have pushed two commit to your branch

  1. typo fix to execute real dotnet-format build
  2. drop installation for obsolete tool.

Please check what you can do with detected issues https://github.com/open-telemetry/opentelemetry-dotnet-contrib/actions/runs/5676698870/job/15383851038?pr=1276

@ErtanTaner ErtanTaner force-pushed the build/change-formatter branch from 3c71621 to f9f0585 Compare July 28, 2023 00:15
@ErtanTaner
Copy link
Author

ErtanTaner commented Jul 28, 2023

Branch was not up to date with the current changes and ı accidently force pushed. That's remove your commits, ı am so sorry about it. Going to make those changes again.

@ErtanTaner ErtanTaner force-pushed the build/change-formatter branch from 0c8693d to d93825f Compare August 1, 2023 17:17
@ErtanTaner
Copy link
Author

@Kielek @utpilla I tried to use format without errors in my machine and it's only works if ı build before the format process. The old one (dotnet-format) don't need this. Idk what is related to and how could ı fix it. Should ı add a build step in yml or are we going to use the old one ?

@Kielek
Copy link
Contributor

Kielek commented Aug 2, 2023

I see a lot of issues similar to

Required references did not load for OpenTelemetry.Contrib.Tests.Shared or referenced project. Run `dotnet restore` prior to formatting.

I think that you have 2 options to proceed

  1. Add separate step which will restore packages before current one
  2. Remove --no-restore from current command.

I think the first option should be better to have separate time measurement for just code formatting.

Keeping the old version is not good idea - it is not supporting new C# futures so it will prevent us using it.

@ErtanTaner ErtanTaner force-pushed the build/change-formatter branch from d93825f to e3de1fe Compare August 3, 2023 17:14
@ErtanTaner ErtanTaner force-pushed the build/change-formatter branch from ef76684 to 38dc6cc Compare August 7, 2023 17:03
@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Merging #1276 (38dc6cc) into main (e0eeb0a) will increase coverage by 0.29%.
Report is 7 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1276      +/-   ##
==========================================
+ Coverage   73.83%   74.13%   +0.29%     
==========================================
  Files         264      256       -8     
  Lines        9386     9344      -42     
==========================================
- Hits         6930     6927       -3     
+ Misses       2456     2417      -39     
Files Changed Coverage Δ
...ry.Exporter.Geneva/TLDExporter/TldTraceExporter.cs 32.31% <ø> (ø)
...on.AWS/Implementation/AWSTracingPipelineHandler.cs 87.75% <ø> (ø)
...tion.AWS/Implementation/SnsRequestContextHelper.cs 95.00% <ø> (ø)
...tion.AWS/Implementation/SqsRequestContextHelper.cs 95.00% <ø> (ø)
...ntation.AWSLambda/Implementation/AWSLambdaUtils.cs 81.57% <ø> (ø)
...ResourceDetectors.Azure/AzureVmMetadataResponse.cs 94.28% <ø> (ø)
src/OpenTelemetry.Sampler.AWS/RateLimiter.cs 100.00% <ø> (ø)
...c/OpenTelemetry.Sampler.AWS/RateLimitingSampler.cs 100.00% <ø> (ø)
...Lambda/Implementation/AWSLambdaResourceDetector.cs 100.00% <100.00%> (ø)
...ation.AWSLambda/TracerProviderBuilderExtensions.cs 100.00% <100.00%> (ø)

... and 8 files with indirect coverage changes

@ErtanTaner
Copy link
Author

@Kielek Like ı said, ı tried to use format in my computer and without dotnet build, still gave the error like the last checks. Should ı include a dotnet build step or is there any problem am ı missing because ı already added restore step to yml file?

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Aug 15, 2023
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra Infra work - CI/CD, code coverage, linters Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace outdated tool dotnet-format by the built-in dotnet format
4 participants