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

Factored out Contants.*** Role into RoleNames.*** #805

Merged
merged 6 commits into from
Oct 18, 2020
Merged

Factored out Contants.*** Role into RoleNames.*** #805

merged 6 commits into from
Oct 18, 2020

Conversation

TonyValenti
Copy link

Renamed 'AllUsers' to 'Everyone'

Renamed 'AllUsers' to 'Everyone'
@dnfadmin
Copy link

dnfadmin commented Oct 16, 2020

CLA assistant check
All CLA requirements met.

@mikecasas
Copy link
Contributor

I'm not against refactoring's like this, but it's going to break all my external modules. I know you included the Obsolete attribute.

If this is part of the movement to .NET 5, then I am ok with it.

My external controller code:

// GET api/<controller>/5
[HttpGet("{id}")]
[Authorize(Roles = Constants.RegisteredRole)]
public ReportViewer Get(int id)
{
  return _ReportViewers.GetReportViewer(id);
}

@TonyValenti
Copy link
Author

@mikecasas What could I do different to make it not break?
Is it just breaking because it treats warnings as errors?

@mikecasas
Copy link
Contributor

I meant it more in line that I would have to go and change all my modules - which I am not opposed to. It would be a simple find and replace.

I guess I just don't see the benefit - but that's me.

@hishamco
Copy link
Contributor

The PR title is something and "Renamed 'AllUsers' to 'Everyone'" is another thing, could please filed an issue to describe the problem before you invest a time of something may not acceptable

Thanks

@sbwalker
Copy link
Member

@mikecasas I am reviewing this PR and based on the fact that the current constants were not removed, I do not see where the breaking change is - are you able to explain? A breaking change is when your module throws a run-time error due to an incompatibility introduced in the framework. If what you are referring to is compilation warnings based on the Obsolete attributes it is not a breaking change as your module is still functional without any changes.

@mikecasas
Copy link
Contributor

I mis-spoke, I should not have used the term breaking change.

@TonyValenti
Copy link
Author

https://docs.microsoft.com/en-us/dotnet/core/compatibility/

This article has a good description of breaking changes. I don’t believe the changes I made are breaking.

Additionally, I would suggest adding the above article to the philosophy article and making that part of the official documentation in a markdown file.

@sbwalker sbwalker merged commit 9543cd7 into oqtane:master Oct 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants