Skip to content

Add Ocelot.Common Project to the solution#1671

Closed
ggnaegi wants to merge 5 commits intoThreeMammals:developfrom
ggnaegi:feature/ocelot-common
Closed

Add Ocelot.Common Project to the solution#1671
ggnaegi wants to merge 5 commits intoThreeMammals:developfrom
ggnaegi:feature/ocelot-common

Conversation

@ggnaegi
Copy link
Copy Markdown
Collaborator

@ggnaegi ggnaegi commented Jun 27, 2023

New Feature #

It would be great to have a common project, addressing crosscutting concerns (standardized exceptions, string constants etc, maybe some factories...).

Proposed Changes

Create a new Ocelot.Common project
Add Constants, eg. "ServiceFabric" to a static class, "Constants"

Guillaume Gnaegi added 5 commits June 20, 2023 16:17
fixing some issues in poll consul:
1c78085 
- Timer is not thread safe, avoiding usage of it
- No Ressources are returned for first call
- Using a providers pool, instead of creating a new provider instance
line endings
ad39599
adding some test cases
99590a9
Using a lock instead of SemaphoreSlim
53d0c7f
It's probably a good idea to have a common project. Adding a Constant…
4d9172f 
…s class for commonly used constants, such as "ServiceFabric"
@ggnaegi ggnaegi changed the title Feature/ocelot common Add Ocelot.Common Project to the solution Jun 27, 2023
@ggnaegi
Copy link
Copy Markdown
Collaborator Author

ggnaegi commented Jun 27, 2023

@raman-m well, so I forgot to switch back to the develop branch...

@raman-m
Copy link
Copy Markdown
Member

raman-m commented Jun 28, 2023

Well... I have a mixed feelings regarding this PR...

It seems you are trying to propose some new feature right in the PR. It doesn't, and it should not work.
You need to create an issue first and start discussions for acceptance.

Regarding this PR...
You are trying to show some idea with common project based on the code from develop. But you would like to improve the code from #1670 , right? I see a lot of code from the #1670 PR!
In this case you have to branch from bug/polling-consul branch. And prepare new changes to merge to bug/polling-consul! That's how to see your new proposed changes.
Finally, if #1670 will be merged you will be able to re-target to the develop branch.
Do you see?

Sorry, I'm going to make this PR as draft...
I don't understand your idea right now.

@raman-m raman-m marked this pull request as draft June 28, 2023 17:17
@raman-m
Copy link
Copy Markdown
Member

raman-m commented Jun 28, 2023

line endings

And do not change any line endings at all please!
Such changes have just zero outcome!

Make new PR in your forked repo plz, and I will review it if you wish.

@raman-m raman-m closed this Jun 28, 2023
@raman-m raman-m added invalid Not actually an issue wontfix No plan to include this issue in the Ocelot core functionality. labels Jun 28, 2023
@ggnaegi
Copy link
Copy Markdown
Collaborator Author

ggnaegi commented Jun 28, 2023

@raman-m Yes, I made a mistake there. But, there is a reason why. For me, using string litterals for comparison is a code smell. I have several examples like, "ServiceFabric" and others. I will make a proposal, you're right.

@raman-m
Copy link
Copy Markdown
Member

raman-m commented Jun 29, 2023

A soon as you've created new PR in your forked repo, then link both of PRs please: this PR to the new one.
Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@ggnaegi ggnaegi

Labels

invalid Not actually an issue wontfix No plan to include this issue in the Ocelot core functionality.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ggnaegi @raman-m