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

Added missing @Deprecated annotations #5209

Merged
merged 1 commit into from
Jan 14, 2023
Merged

Added missing @Deprecated annotations #5209

merged 1 commit into from
Jan 14, 2023

Conversation

tbw777
Copy link
Contributor

@tbw777 tbw777 commented Jan 5, 2023

These annotations helpfully to prevent usage of deprecated API. For example: developer can see crossed out method/field name if this item is deprecated.

@mbien mbien added Code cleanup ci:all-tests [ci] enable all tests labels Jan 6, 2023
@apache apache locked and limited conversation to collaborators Jan 6, 2023
@apache apache unlocked this conversation Jan 6, 2023
@mbien mbien added this to the NB17 milestone Jan 9, 2023
@mbien
Copy link
Member

mbien commented Jan 9, 2023

@neilcsmith-net this one might be a candidate for NB 17, what do you think?

@neilcsmith-net
Copy link
Member

@mbien in principle should be safe. Will also add into the sig files / tests. Your call!

If we're sure code cleanup changes have no behavioural changes (some of these PR do) then probably safe to merge even this close to branch. Others I would prefer we push until after NB17 is released - some could cause some "fun" with sync PRs?

@mbien
Copy link
Member

mbien commented Jan 9, 2023

@neilcsmith-net ok i haven't considered sig tests causing trouble - good that you mention that. I was only thinking about this PR here with the deprecation annotations - I just removed all other cleanup PRs from milestone 17.

Also.. the sig test seems to throw [sigtest] java.lang.ClassFormatError: Index out of the constant pool bounds which it probably shouldn't do.

@mbien
Copy link
Member

mbien commented Jan 11, 2023

@ebarboni you did recently a lot of doc related work and know also a lot more about the NB build than me. You think adding the deprecation annotation would cause any issues anywhere? I think this would be an important PR to be merged.

(yes it will cause more compiler warnings, but this is intended.)

@mbien mbien requested a review from ebarboni January 11, 2023 05:34
@mbien
Copy link
Member

mbien commented Jan 14, 2023

@matthiasblaesing do you see any potential issues caused by bulk adding the deprecated annotation for already in-doc deprecated items? (beside more compiler warnings)

I can't come up with a good reason to not merge this for NB17.

Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

In general ok, but there are problems (see inline comments).

For the point Neil made, this is the result (example for editor.mimelookup):

# This patch file was generated by NetBeans IDE
# It uses platform neutral UTF-8 encoding and \n newlines.
--- a/platform/editor.mimelookup/nbproject/org-netbeans-modules-editor-mimelookup.sig
+++ b/platform/editor.mimelookup/nbproject/org-netbeans-modules-editor-mimelookup.sig
@@ -1,5 +1,5 @@
 #Signature file v4.1
-#Version 1.58
+#Version 1.59
 
 CLSS public abstract interface !annotation java.lang.Deprecated
  anno 0 java.lang.annotation.Documented()
@@ -49,9 +49,13 @@
 
 CLSS public final org.netbeans.api.editor.mimelookup.MimeLookup
 meth public <%0 extends java.lang.Object> org.openide.util.Lookup$Result<{%%0}> lookup(org.openide.util.Lookup$Template<{%%0}>)
+ anno 0 java.lang.Deprecated()
 meth public <%0 extends java.lang.Object> {%%0} lookup(java.lang.Class<{%%0}>)
+ anno 0 java.lang.Deprecated()
 meth public org.netbeans.api.editor.mimelookup.MimeLookup childLookup(java.lang.String)
+ anno 0 java.lang.Deprecated()
 meth public static org.netbeans.api.editor.mimelookup.MimeLookup getMimeLookup(java.lang.String)
+ anno 0 java.lang.Deprecated()
 meth public static org.openide.util.Lookup getLookup(java.lang.String)
 meth public static org.openide.util.Lookup getLookup(org.netbeans.api.editor.mimelookup.MimePath)
 supr org.openide.util.Lookup

The anntoation is recorded in the signature, but I don't see a problem with that. Even when branching that should not be an issue. The additional @Deprecated annotations might cause more merge conflicts, but that would be true for any large scale cleanup. The size here seems doable.

@tbw777
Copy link
Contributor Author

tbw777 commented Jan 14, 2023

fixed

@tbw777 tbw777 requested review from matthiasblaesing and removed request for lkishalmi and ebarboni January 14, 2023 16:28
@tbw777 tbw777 requested review from mbien and removed request for matthiasblaesing January 14, 2023 17:00
Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

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

marked one formatting adjustment. After that please squash everything to one commit.

If you need instructions how to squash please say so I can help.

These annotations helpfully to prevent usage of deprecated API. For example: developer can see crossed out method/field name if this item is deprecated.
@tbw777 tbw777 requested a review from mbien January 14, 2023 18:10
Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

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

thanks. looked through it again and it looks good to me.

Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Looks sane to me.

@mbien mbien merged commit 6302f1c into apache:master Jan 14, 2023
@tbw777 tbw777 deleted the Deprecated branch January 14, 2023 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:all-tests [ci] enable all tests Code cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants