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

Strengthen EditorConfig to help enforce coding standards #775

Merged
merged 16 commits into from
Dec 7, 2023

Conversation

ChrisPulman
Copy link
Member

Update code to match updated coding standard.

Produces errors when coding doesn't match styling and recommended C# rules.

@JakenVeina
Copy link
Collaborator

to match updated coding standard

What's the coding standard that we follow here?

@ChrisPulman
Copy link
Member Author

to match updated coding standard

What's the coding standard that we follow here?

StyleCop + .editorconfig rules, it's the same set of rules that we run with the other ReactiveMarbles and ReactiveUI projects. The main alterations are ensuring that code is maintained in order of modifier and making suggestions for recommended changes. Errors are raised when there's code that doesn't meet the core requirements.

@glennawatson
Copy link
Member

Based on the dotnet runtime settings. Documented on the rxui website.

@RolandPheasant
Copy link
Collaborator

Let's get outstanding PRs in before this, otherwise we'll get some merge conflicts.

Reduce code bloat by using extension method for ArgumentNullException's. This was chosen due to current target frameworks. If in the future netstandard2.1 + compliant frameworks are used we can switch methods.
@ChrisPulman ChrisPulman marked this pull request as draft December 4, 2023 19:56
@ChrisPulman ChrisPulman marked this pull request as ready for review December 5, 2023 20:17
@RolandPheasant
Copy link
Collaborator

I love seeing PRs with these kind of stats

image

src\DynamicData.Tests\Cache\TransformFixtureParallel.cs line 95 - Test Needs fixing or updated Functionality needs checking
@ChrisPulman
Copy link
Member Author

@JakenVeina src\DynamicData.Tests\Cache\TransformFixtureParallel.cs line 95 - Test Needs fixing or updated Functionality needs checking after PR #771

I have Skipped this test for the moment to ensure that there's no other tests with issues.
Do you think you could check this test to verify it?
Running this test as a single test on your machine with "run until failure" should replicate the issue.

Test - src\DynamicData.Tests\Cache\TransformFixtureParallel.cs SameKeyChanges on line 95 needs attention
@ChrisPulman
Copy link
Member Author

Skip has been removed to invoke test failure - awaiting Test update or core functionality update.
@RolandPheasant If you still wish to commit this prior to the investigation or update then we should add the skip back in, the test passes on a 2 in 10 basis i.e. its random

@ChrisPulman
Copy link
Member Author

Running the same test on Main doesn't produce the same issue, so perhaps there is a merge issue somewhere.

@RolandPheasant
Copy link
Collaborator

RolandPheasant commented Dec 7, 2023

Skip has been removed to invoke test failure - awaiting Test update or core functionality update. @RolandPheasant If you still wish to commit this prior to the investigation or update then we should add the skip back in, the test passes on a 2 in 10 basis i.e. its random

There's nothing wrong with the code, and nothing that any user has ever complained about in almost 10 years. The issue I think is purely down to mixing paradigms, and making tests work. Parallel.for sucks, but seemed a good idea at the time. Possibly in the future I'll change it to use pure Rx.

I am happy for the test to be skipped.

@RolandPheasant
Copy link
Collaborator

Running the same test on Main doesn't produce the same issue, so perhaps there is a merge issue somewhere.

In which case, maybe my above comment is complete rubbish

@ChrisPulman
Copy link
Member Author

Running the same test on Main doesn't produce the same issue, so perhaps there is a merge issue somewhere.

In which case, maybe my above comment is complete rubbish

No problem, I think there must be something in the auto merge that happened, it's 05:43 here I better go back to sleep :)
If @JakenVeina doesn't manage to find the issue before tomorrow evening, I will try to see what went wrong, I return to UK on Sunday so will be in the Air for 12 hours.

@RolandPheasant
Copy link
Collaborator

@ChrisPulman thanks for helping and have a good journey. I am in the air too tomorrow.

@JakenVeina
Copy link
Collaborator

JakenVeina commented Dec 7, 2023

So the issue is not coming from #771, it's coming from the merge commit right after that, 3a39aac. Rolling back that commit eliminates the issue, with #771 still present on the branch. The issue is also not present on main, which is f492247 at the moment.

It looks like that merge pulled with it 11 other commits, so I'll see if I can narrow it down further.

It looks like this issue was present on the branch before #771 was merged. Commit 6395e7d seems to be the source, rolling back to just before that commit eliminates the issue, and pulling just that commit back in reintroduces it.

@JakenVeina
Copy link
Collaborator

Found it.

@RolandPheasant RolandPheasant merged commit 65bb022 into main Dec 7, 2023
1 check passed
@RolandPheasant RolandPheasant deleted the CP_EnhanceCodeFormatingRequirements branch December 7, 2023 12:52
@ChrisPulman
Copy link
Member Author

Found it.

Thank you very much @JakenVeina

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants