Skip to content

refactor: replace Nett with Tomlyn #230

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Sep 7, 2022
Merged

refactor: replace Nett with Tomlyn #230

merged 15 commits into from
Sep 7, 2022

Conversation

@Omotola
Copy link
Contributor

@Omotola Omotola commented Aug 26, 2022

PR for Issue #184

@Omotola Omotola requested a review from a team as a code owner August 26, 2022 17:53
@Omotola Omotola requested a review from JamieMagee August 26, 2022 17:53
Omotola
Omotola commented Aug 26, 2022
View reviewed changes
src/Microsoft.ComponentDetection.Detectors/poetry/Contracts/PoetryPackage.cs Outdated
[SuppressMessage("StyleCop.CSharp.NamingRules", "SA1300:ElementMustBeginWithUpperCaseLetter", Justification = "Deserialization contract. Casing cannot be overwritten.")]
public PoetrySource source { get; set; }

public TomlTable dependencies { get; set; }
Copy link
Contributor Author

@Omotola Omotola Aug 26, 2022

Choose a reason for hiding this comment

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

Had to add this property because tomlyn wouldn't create the model if the property didn't exist and their "TomlModelOptions.IgnoreMissingProperties" didnt work in this scenario

@JamieMagee JamieMagee changed the title Users/oakeredolu/nett tomlyn refactor: replace Nett with Tomlyn Aug 26, 2022
JamieMagee
JamieMagee reviewed Aug 26, 2022
View reviewed changes
src/Microsoft.ComponentDetection.Detectors/poetry/Contracts/PoetryPackage.cs Outdated

public TomlTable dependencies { get; set; }

public TomlTable extras { get; set; }
Copy link
Member

@JamieMagee JamieMagee Aug 26, 2022

Choose a reason for hiding this comment

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

This needs a [DataMember(Name = "extras")] attribute.

Suggested change
public TomlTable extras { get; set; }
[DataMember(Name = "extras")]
public TomlTable Extras { get; set; }

@github-actions
Copy link

github-actions bot commented Aug 26, 2022

👋 Hi! It looks like you modified some files in the Detectors folder.
You may need to bump the detector versions if any of the following scenarios apply:

  • The detector detects more or fewer components than before
  • The detector generates different parent/child graph relationships than before
  • The detector generates different devDependencies values than before

If none of the above scenarios apply, feel free to ignore this comment 🙂

JamieMagee
JamieMagee previously requested changes Aug 26, 2022
View reviewed changes
Copy link
Member

@JamieMagee JamieMagee left a comment

Choose a reason for hiding this comment

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

Can you:

  • Resolve the build warnings
  • Manually run this against some of our test cases in the repo and check it works okay

@cobya cobya added type:refactor Refactoring or improving of existing code dependencies Pull requests that update a dependency file labels Sep 7, 2022
@Omotola Omotola enabled auto-merge (squash) September 7, 2022 18:05
RushabhBhansali
RushabhBhansali reviewed Sep 7, 2022
View reviewed changes
docs/running-verification-tests.md Outdated


## Step 4: Run The tests
Run or Debug the tests in test explorer like you would any other test. No newline at end of file
Copy link
Contributor

@RushabhBhansali RushabhBhansali Sep 7, 2022

Choose a reason for hiding this comment

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

add or run dotnet test from /Verification test folder from commandLine to execute them

RushabhBhansali
RushabhBhansali reviewed Sep 7, 2022
View reviewed changes
src/Microsoft.ComponentDetection.Detectors/poetry/PoetryComponentDetector.cs Outdated

namespace Microsoft.ComponentDetection.Detectors.Poetry
{
using System.IO;
Copy link
Contributor

@RushabhBhansali RushabhBhansali Sep 7, 2022

Choose a reason for hiding this comment

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

shift these 2 using statements outside of namespace alogn with other using statements.

RushabhBhansali
RushabhBhansali approved these changes Sep 7, 2022
View reviewed changes
src/Microsoft.ComponentDetection.Detectors/rust/Contracts/CargoLock.cs

namespace Microsoft.ComponentDetection.Detectors.Rust.Contracts
{
using System.Collections.Generic;
Copy link
Contributor

@RushabhBhansali RushabhBhansali Sep 7, 2022

Choose a reason for hiding this comment

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

pout these statements outside of namespace at class level.

Copy link
Contributor

@RushabhBhansali RushabhBhansali Sep 7, 2022

Choose a reason for hiding this comment

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

I would apply "Convert to file-scoped namespace" at the same time, too. Also, remove unneccessary directives.

@Omotola Omotola dismissed JamieMagee’s stale review September 7, 2022 19:40

Resolved the suggestions, but Jamie is not here to approve

@Omotola Omotola merged commit 65463b4 into main Sep 7, 2022
@Omotola Omotola deleted the users/oakeredolu/nett-tomlyn branch September 7, 2022 19:40
@JamieMagee JamieMagee mentioned this pull request Oct 17, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JamieMagee JamieMagee Awaiting requested review from JamieMagee

@RushabhBhansali RushabhBhansali Awaiting requested review from RushabhBhansali

Assignees

@RushabhBhansali RushabhBhansali

Labels

dependencies Pull requests that update a dependency file type:refactor Refactoring or improving of existing code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Omotola @JamieMagee @RushabhBhansali @cobya