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

Counter aspect changes #132

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Rezix93
Copy link

@Rezix93 Rezix93 commented Jul 31, 2024

No description provided.

@arfio arfio self-requested a review July 31, 2024 20:55
*/
protected void handleGroupedCounterAspect(ITmfEvent event, ITmfStateSystemBuilder ss, CounterAspect aspect, int rootQuark) {
protected void handleGroupedCounterAspect(ITmfEvent event, ITmfStateSystemBuilder ss, ITmfCounterAspect aspect, int rootQuark) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is detected as major API change. I think you have to keep the old signature and deprecated it, and have it call the new signature, casting the aspect to avoid recursion.

With this change, it will only require a minor version bump to 2.3.0, this must be done in the plug-in MANIFEST.MF and will affect all updated @SInCE tags in this commit, to be set to 2.3.

@@ -81,6 +81,7 @@ public CounterAspect(String fieldName, String label, CounterType type, Class<? e
*
* @return the groups
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that it overrides, you can remove the Javadoc here, the one from interface will be used.

Copy link
Contributor

Choose a reason for hiding this comment

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

not fixed

@@ -130,6 +130,7 @@ public boolean equals(@Nullable Object obj) {
* @return the type of this counter
* @since 2.1
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that it overrides, you can remove the Javadoc here, the one from interface will be used.

Copy link
Contributor

Choose a reason for hiding this comment

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

not fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the Javadoc of getType() that can be removed.

* @return the groups
* @since 2.2
*/
default Class<? extends ITmfEventAspect<?>>[] getGroups(){
Copy link
Contributor

Choose a reason for hiding this comment

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

space before {

Copy link
Contributor

Choose a reason for hiding this comment

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

not fixed

@Rezix93 Rezix93 force-pushed the counterAspectChanges branch 2 times, most recently from 0a8a421 to 7ac254b Compare August 2, 2024 13:59
* @return the groups
* @since 2.2
*/
default Class<? extends ITmfEventAspect<?>>[] getGroups(){
Copy link
Contributor

Choose a reason for hiding this comment

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

not fixed

@@ -81,6 +81,7 @@ public CounterAspect(String fieldName, String label, CounterType type, Class<? e
*
* @return the groups
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

not fixed

@@ -130,6 +130,7 @@ public boolean equals(@Nullable Object obj) {
* @return the type of this counter
* @since 2.1
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

not fixed

* Key to a relative root of the state system
* @param aspect
* Grouped counter aspect
* @since 2.2
Copy link
Contributor

Choose a reason for hiding this comment

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

API change requires bump to 2.3.0 in MANIFEST.MF and since tag changed to 2.3 here.

* Get the groups
*
* @return the groups
* @since 2.2
Copy link
Contributor

Choose a reason for hiding this comment

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

API change requires bump to 2.3.0 in MANIFEST.MF and since tag changed to 2.3 here.

Copy link
Contributor

Choose a reason for hiding this comment

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

2.3

* Gets the type of this counter
*
* @return the type of this counter
* @since 2.2
Copy link
Contributor

Choose a reason for hiding this comment

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

API change requires bump to 2.3.0 in MANIFEST.MF and since tag changed to 2.3 here.

Copy link
Contributor

Choose a reason for hiding this comment

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

2.3

Copy link
Contributor

Choose a reason for hiding this comment

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

also missing a @return tag

@Rezix93 Rezix93 force-pushed the counterAspectChanges branch 2 times, most recently from 87a6529 to 1482a21 Compare August 2, 2024 16:40
* {@inheritDoc}
*
* This is a conservative equals. It only works on very identical aspects.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this one should stay, it extends the inherited Javadoc.

@@ -130,6 +130,7 @@ public boolean equals(@Nullable Object obj) {
* @return the type of this counter
* @since 2.1
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

It's the Javadoc of getType() that can be removed.

* Get the groups
*
* @return the groups
* @since 2.2
Copy link
Contributor

Choose a reason for hiding this comment

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

2.3

* Gets the type of this counter
*
* @return the type of this counter
* @since 2.2
Copy link
Contributor

Choose a reason for hiding this comment

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

2.3

* Gets the type of this counter
*
* @return the type of this counter
* @since 2.2
Copy link
Contributor

Choose a reason for hiding this comment

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

also missing a @return tag

[Changed] Allowed ITmfCounterAspect populate counterAnalysis

Signed-off-by: Reza Rouhghalandari <[email protected]>
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.

2 participants