Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Number of open issue probe #353
base: main
Are you sure you want to change the base?
Number of open issue probe #353
Changes from 44 commits
2773dcf
68aa505
54a82a3
abb1001
4d8ae5d
c4c1605
99d9cf5
719c57d
925afa3
58b135a
c71a32a
5fd8b2d
6f9ede3
1a54932
4a144e5
d3fd18b
d960448
eda1b13
97aed28
528ab43
5f22f38
e13f3a8
9bcb49e
4989e81
7db7e7a
e45ce77
510d340
3cf7406
b9cda9c
37884d5
898c86c
fdd1a4e
1940e67
151df0d
12ef4aa
7f6cb04
6a7214a
8e0f12a
12db24b
793114b
e13b17e
f4ef76e
1e194e9
d9fad0c
663282c
c099e40
ec28408
6a740d2
9244d60
b722a87
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Jagrutiti,
There is something wrong with that class which comes from its design.
On purpose, I won't give you more indication for now, to let you figure it out by yourself.
Please have a look at other similar classes in the project, and please read more about the purpose of
abstract
classes, and the general purpose of abstraction in OOP.You already implemented similar kind of classes in previous PRs, but perhaps the extra guidance we provided made that you did not get to understand the reason why we recommended this design, which is why this time I won't share much details (yet) and let you take some time to think about the design choices and understand the reason for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @aneveux ,
I did read about implementing an
abstract
class in OOPS.The articles spoke of data abstraction and functional abstraction. In our case I believe, we are doing functional abstraction.
Having two different methods is important here because the implementation to count issues from both
JIRA
andGitHub
have nothing in common.I had done a similar implementation in
AbstractDependencyBotConfigurationProbe
but in that caseRenovateProbe
andDependabotProbe
had the same job. Look for files only with different names.The next point that I come across is why
abstract
classes are used. They are designed for:The abstract classes that I implemented did provide code reusability and their functionalities were hidden from the child class. They only had to implement the abstract class.
In the
NumberofOpenIssuesProbe
code reusability is not possible because the libraries and API used in both cases are different.About the design issues,
JIRA_HOST
andrestTemplate
are used in only case ofJIRA
issues.Maybe I should change the
JIRA_HOST
toHOST_URL
and that should be assigned by all the sub-classes.Maybe
JIRA_HOST
andrestTemplate
should be assigned using a constructor in the subclasses?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me just say, what if tomorrow there is a third issue tracker used in the Jenkins project, what would we have to add or changed in here to make it fit?
In OOP, it should require a minimal addition, very specific.
If you look at the DependencyBot abstract class, how are the different bot handled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Jagrutiti 👋
Sorry for the late reply, I'm out of the office right now and spend less time on the computer 😝
Adrien actually gave a nice indication regarding the design: what happens if you had to add a third issue tracker?
The idea behind code abstraction is that the
abstract
class that you write should not know at all about its possible implementations. In your case, not only yourabstract
class knows about Jira and GitHub, but it even provides code for it.An
abstract
class should contain all the code that is supposed to be common between all of its subclasses, and no more. If there is some code that is used by only one of the subclasses, it is rarely a good fit for theabstract
class.That being said though, maybe while implementing your code you will notice that there is nothing to share between the two classes that you write because they are too different from each other, and in that case, it would be better to challenge the need for abstraction, and simply write 2 classes that are doing their job.
Introducing abstraction just for the sake of it just makes the source code more complicated, hence less readable.
It is tempting to want to use good practices like abstraction while writing code when we manipulate things that seem similar, but it is important to check that they are indeed similar.
Hope that helps you understand my concern with the class design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for explaining @aneveux.
In past cases when I used
abstract
classes. 99% of the code was common.In this case, there was no common code. Maybe I should have asked this in the meeting itself but it did not occur to me then.
I should have taken inspiration from
doApply
abstract method inProbe
class. But I did not understand the "why" part until I made this blunder.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's alright @Jagrutiti =) We learn by making mistakes, and if you never experienced such a case before, it would have been difficult to think about it and ask a question during the meeting.
If you understand it better now, then that's great =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
context.getIssueTrackerUrlsByNames()
cannot benull
right?If you are not sure, you can add a first check to validate that the plugin has an issue tracker configured? (a simple
if
before trying to.get("github");
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IssueTrackerDetectionProbe
is the requirement forGitHubOpenIssuesProbe
andUpdateCenterPluginPublicationProbe
is the requirement forIssueTrackerDetectionProbe
that is why a part of my believes that this issue might not occur.It is better to be safe than sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the "better safe than sorry" principal. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel those should go in the
getCountOfOpenIssues
method and not in the class itself. They are not meant to be reused by something else in the class right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree here. those fields should be local variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot mock the variables if they are not global in the test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. Let me look into this because this doesn't seem right to create class fields just because of the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
context.getIssueTrackerUrlsByNames()
cannot benull
right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before, "better safe than sorry".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
if
condition checks whether thenull
values below this line.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same remark: which plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this was addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires the
plugin
to be passed as a parameter togetCountOfOpenIssues
to getplugin.getName()
that is whydid not work on this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or you could just provide the
name
of the plugin, rather than the entireplugin
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did something similar last time. You said we shouldn't pass a parameter just to display in the logs, But if you are cool with it I can do that,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because
URL#equals
method is doing a DNS resolution, I'd prefer usingURI
class which provide the same features but with less problems.