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

[NETBEANS-5661] - Added regular expression window and hint #2953

Merged
merged 1 commit into from
Jul 20, 2021

Conversation

mishrasandeep
Copy link
Contributor

@mishrasandeep mishrasandeep commented May 13, 2021

https://issues.apache.org/jira/browse/NETBEANS-5661 Adding a feature for testing the valid values for a Regular Expression in the IDE itself.

  • Added the window to the Menu. Can be accessed from Menu/Window without context.
  • Can be reference with shortcut Ctrl + 8 (not being used for any other action)
  • Added strict check option to disable checking for substring values matching the regular expression.

Modified "Malformed Regexp" (shows invalid regular expression description) Hint to severity error instead of warning and scan for value of the identifiers.

@mishrasandeep mishrasandeep changed the title Added regular expression window and hint NETBEANS-5661 - Added regular expression window and hint May 13, 2021
@mishrasandeep mishrasandeep marked this pull request as ready for review June 1, 2021 04:51
Copy link
Member

@junichi11 junichi11 left a comment

Choose a reason for hiding this comment

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

I reviewed only code style.

@jkovalsky jkovalsky self-requested a review June 15, 2021 11:23
Copy link
Contributor

@jkovalsky jkovalsky left a comment

Choose a reason for hiding this comment

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

I have provided some minor comments for your code but in general it looks good to me. Before approving this PR I would recommend though to:

  • rename the UI widgets to meaningful names (not jLabel1 etc.) as @junichi11 already mentioned above
  • listen to keystrokes also in the jTextArea1 ;) so that if one modifies the regular expression it immediately reevaluates the example
  • take a look at failed unit tests to avoid unwanted regressions

@junichi11
Copy link
Member

@mishrasandeep
Thank you for your contribution!
Could you please squash as one commit with a proper commit message after you fix all? (We don't use the squash and merge button.)

Copy link
Contributor

@singh-akhilesh singh-akhilesh left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Akshay-Gupta-19 Akshay-Gupta-19 left a comment

Choose a reason for hiding this comment

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

Looks correct to me.

@junichi11 junichi11 added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) Need Squashing labels Jun 24, 2021
Copy link
Contributor

@jkovalsky jkovalsky left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my previous comments. I have provided some more findings. Otherwise I am fine with the work and approve it.

@mishrasandeep mishrasandeep changed the title NETBEANS-5661 - Added regular expression window and hint [NETBEANS-5661] - Added regular expression window and hint Jul 1, 2021
@mishrasandeep
Copy link
Contributor Author

mishrasandeep commented Jul 2, 2021

@mishrasandeep
Thank you for your contribution!
Could you please squash as one commit with a proper commit message after you fix all? (We don't use the squash and merge button.)

@junichi11
Have addressed all the comments and squashed the commits. Any other inputs from your end?

Copy link
Member

@junichi11 junichi11 left a comment

Choose a reason for hiding this comment

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

Could you please check to format (ranges of your changes) again? (I didn't take a look at all files.)

@junichi11
Copy link
Member

Maybe, We should also update this file: https://github.com/apache/netbeans/blob/master/java/java.hints/licenseinfo.xml

@matthiasblaesing Aren't the license of png files checked via CI(RAT)?

@mishrasandeep
Copy link
Contributor Author

Maybe, We should also update this file: https://github.com/apache/netbeans/blob/master/java/java.hints/licenseinfo.xml

@matthiasblaesing Aren't the license of png files checked via CI(RAT)?

@junichi11 should I go ahead and update the file?

@junichi11
Copy link
Member

should I go ahead and update the file?

@mishrasandeep Yes. Let's update it for your new png files.

@mishrasandeep
Copy link
Contributor Author

should I go ahead and update the file?

@mishrasandeep Yes. Let's update it for your new png files.

@junichi11 have updated the license file

@sdedic
Copy link
Member

sdedic commented Jul 9, 2021

Is it possible to include this in 12.5 ?

Copy link
Member

@junichi11 junichi11 left a comment

Choose a reason for hiding this comment

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

@mishrasandeep Thank you for fixing it. I reviewed only code style.
Could you please squash again?

@junichi11
Copy link
Member

@sdedic @jkovalsky @Akshay-Gupta-Oracle @singh-akhilesh
If it's OK for you please feel free to merge this after commits are squashed. (Is the Travis error OK?)

@sdedic
Copy link
Member

sdedic commented Jul 9, 2021

Is the Travis error OK?

Should be - it's a timeout (happens sometimes).

@ebarboni
Copy link
Contributor

ebarboni commented Jul 9, 2021

Should be - it's a timeout (happens sometimes).

Too often :D, Let's merge this

@ebarboni ebarboni merged commit ea9ebc6 into apache:master Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) Need Squashing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants