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

#1681 'using' statements life hacks #1682

Merged
merged 16 commits into from
Sep 25, 2023
Merged

#1681 'using' statements life hacks #1682

merged 16 commits into from
Sep 25, 2023

Conversation

raman-m
Copy link
Member

@raman-m raman-m commented Jul 24, 2023

Fixes #1681

Proposed Changes

  • using code block starts at the beginning of file! ⚠️
  • using statements are sorted
  • obsolete, unnecessary using statements are removed
  • added Usings.cs file to define global using statements
  • the number of using statements are decreased to the minimum
  • upgraded xUnit setup in test projects
  • added Implicit using directives to the projects (C# 10). Code Review → But they are disabled in favor of global usings.

@raman-m raman-m added the bug Identified as a potential bug label Jul 24, 2023
@raman-m raman-m self-assigned this Jul 24, 2023
@raman-m
Copy link
Member Author

raman-m commented Jul 24, 2023

@TomPallister Hey Tom!
Could you merge it ASAP please? 🆘
It prevents me from smooth repo support, and it's hard to do quick code reviews now! 🙏 🆘
These merge conflicts are blockers of code review process!
I hope after merging this PR the situation will be better, and we will be able to increase the speed of PR closing. 😥

@raman-m
Copy link
Member Author

raman-m commented Jul 25, 2023

@RaynaldM Hi Ray!
Could you spare a minute please to approve this PR?
The PR doesn't contain real logic. These are workarounds with usings. The goal is minimizing the number of using statements in files to have less probability of merge conflicts in usings block.

@raman-m
Copy link
Member Author

raman-m commented Aug 7, 2023

@RaynaldM discussed on Aug 1

You have answered for me "But I'm not sure, because every junior C# developer knows that global usings are located in some files."
"make short comment line // See global usings in ../Usings.cs" is a good idea.

I've decided not adding this comment line because implicit usings are disabled now and all global usings are presented in this file.
So, this idea looks like overhead one as for me!


In conclusion, I think you can keep ImplicitUsing at true, and just put the little comment "think about looking into GlobalUsing."

Implicit usings are disabled at all. We will use global explicit usings. This is more clear approach to manage usings.


Pay attention that sample projects have implicit usings disabled also, but they won't have Usings.cs file because of small code base. So, there is no sense to add global usings to the sample projects, therefore all using statements will be explicit for samples.

Could you review once again and approve please?

@raman-m raman-m added needs feedback Issue is waiting on feedback before acceptance proposal Proposal for a new functionality in Ocelot labels Aug 7, 2023
@raman-m raman-m requested a review from RaynaldM August 7, 2023 18:57
Copy link
Collaborator

@RaynaldM RaynaldM left a comment

Choose a reason for hiding this comment

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

A lot of files 😊

@raman-m raman-m requested a review from wast August 9, 2023 08:25
@raman-m raman-m added accepted Bug or feature would be accepted as a PR or is being worked on and removed needs feedback Issue is waiting on feedback before acceptance labels Aug 11, 2023
@raman-m
Copy link
Member Author

raman-m commented Aug 11, 2023

@TomPallister
I will appreciate your merging soon!

RaynaldM
RaynaldM previously approved these changes Sep 20, 2023
@raman-m
Copy link
Member Author

raman-m commented Sep 20, 2023

@RaynaldM approved these changes on Sep 20

Thanks for approval! But your approvals are now gray in all PRs! 😱 👇
image

And here is Reviewers block 👇
image

I cannot get it! It seems you are no longer a member of Ocelot team!
@RaynaldM Have you left Ocelot Core team? Or did someone remove you from the team?

@RaynaldM
Copy link
Collaborator

@raman-m, no, I received a message last week telling me that I was no longer part of the team

@raman-m
Copy link
Member Author

raman-m commented Sep 20, 2023

@RaynaldM commented on Sep 20
no, I received a message last week telling me that I was no longer part of the team

Oh, my gosh! I am very sorry!
It seems you were removed from the team by Tom. I am shocked. All your approvals are gray for all PRs 😨
I will figure out why he did that.

Don't worry, I can still add you to the team. Maybe Tom tried to change security policies...

P.S. Please, forward GitHub message aka notification concerning your removal from the team, to me: dotnet044 at gmail dot com

@RaynaldM
Copy link
Collaborator

RaynaldM commented Sep 20, 2023

P.S. Please, forward GitHub message aka notification concerning your removal from the team, to me: dotnet044 at gmail dot com

done

@raman-m
Copy link
Member Author

raman-m commented Sep 21, 2023

@RaynaldM
Welcome to the team!... once again! 😉

RaynaldM
RaynaldM previously approved these changes Sep 25, 2023
RaynaldM
RaynaldM previously approved these changes Sep 25, 2023
@raman-m
Copy link
Member Author

raman-m commented Sep 25, 2023

@RaynaldM Approve once again please!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Bug or feature would be accepted as a PR or is being worked on bug Identified as a potential bug proposal Proposal for a new functionality in Ocelot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unsorted 'using' statements vs merge conflicts
3 participants