Mark duplicate permissions as Obsolete #16176
Conversation
| @@ -6,13 +7,17 @@ namespace OrchardCore.Apis.GraphQL; | |||
|
|
|||
| public class Permissions : IPermissionProvider | |||
| { | |||
|
|
|||
| [Obsolete("This property will be removed in future release. Instead use 'CommonPermissions.ExecuteGraphQLMutations'.")] | |||
There was a problem hiding this comment.
These are not properties, but fields. And more importantly, a reference to the project/package name would help those consumers who don't have the whole of OC referenced:
| [Obsolete("This property will be removed in future release. Instead use 'CommonPermissions.ExecuteGraphQLMutations'.")] | |
| [Obsolete("This field will be removed in a future release. Instead use 'CommonPermissions.ExecuteGraphQLMutations' from OrchardCore.Apis.GraphQL.Abstractions.")] |
Especially because there are many CommonPermissions classes.
Same for all the others.
There was a problem hiding this comment.
field is used when you are declaring a private property. a property when it is public. + Does it really matter? I ask this because these changes consume time for the PR creator to do. During a review, we should ask yourself these questions so that the PR creator does not have to spent lots of time doing changes that add no value. At the end of the day, we want to merge acceptable PR without burden the creators with lots of changes "when possible".
There was a problem hiding this comment.
A field and a property are different language features, and this one here is a field. Both can be public and private (or else) too. I'm only pointing this out because it specified "property"; if it were "This will be removed..." or "This member will be removed.." (which are all alternatives BTW) I wouldn't ask for it to include "field".
You can do a search and replace for this and fix it in seconds though, perhaps less than what it takes to debate this :).
Note the more important second part of my comment about the package though.
There was a problem hiding this comment.
It's not about the debate here. The issues is that we need to think about the value of the comment if it out weight the effort. Many PR end up consuming lots of man hours from (reviewers and creator) due to this type of comment.
I have added the namespaces to the message for developers.
There was a problem hiding this comment.
If it's not about this instance here, then let's take this conversation offline.
src/OrchardCore.Modules/OrchardCore.Search.Elasticsearch/Permissions.cs
Outdated
Show resolved
Hide resolved
|
Why need to make them obsolete, while we're on the |
Just so that developers have a migration path. We can remove these Obsolete attributes in 3.0. |
|
Stange!! I notice there are many obsolete methods already removed since |
Because in the migration from v1 to v2 we instructed users to first update to 1.8 then just to 2.0. This way they resolve all warnings and get their databases to run on 1.8. then they can migrate to v2. Since these changes are part of the 2.0, we should give the users a migrations path. |
|
The migration might be useful but not necessary from within the code base, SemVer all of us know that a major release will introduce breaking changes, it is just a comment, so we can make things right and consider user expectations I spend years using ASP.NET Core and bumping the versions, when it's a major I expected anything I could change in my libraries or apps Maybe, our fault since the beginning when we accept the breaking changes in minor releases. This is a side note but I don't want to disturb the PR |
|
If we'd have a 1.9 with just these changes, and then remove all the obsolete methods in 2.0, then that would be the best. This is the approach every SemVer-following project, including ASP.NET Core, take. However, we can't release 1.9, so this is the next best thing. Having a new major version means we can include breaking changes, not that we should. If we can feasibly make our users' lives easier with these upgrades (including ourselves, maintaining OC apps), then it's better we do that. From 2.0 we can do better. |
| @@ -6,9 +6,10 @@ namespace OrchardCore.Search.Elasticsearch; | |||
|
|
|||
| public class ElasticsearchIndexPermissionHelper | |||
| { | |||
| public static readonly Permission ManageElasticIndexes = new("ManageElasticIndexes", "Manage Elasticsearch Indexes"); | |||
There was a problem hiding this comment.
I was wondering if there was some kind of circular dependency issue or something else hard to see why this was necessary before, but testing Elasticsearch didn't bring up any issues. So, this just looks like an oversight.
There was a problem hiding this comment.
I am guessing this was originally placed here incorrectly and kept that way. Everything passes so I don't see any issue from this.
|
@Piedone but there're a few places that might break such as changing the documents in the database, I closed a few PRs because I was waiting for |
|
I don't really see how that contradicts what I've said. |
I think these static properties were added at some point to provide backward compatibility.
I am marking these properties as obsolete for now, we can then remove them in 3.0 or later.