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
L1 Testing #2806
L1 Testing #2806
Changes from 79 commits
bf63a36
9c587f2
e16b613
fef48d7
e907cf7
3326b4c
2801759
86354c2
3c70225
61f2c0a
b7e5b5e
73a8b1b
b6ff75b
7e07c24
f747c50
cebc7af
a6e66e9
4f9342e
3144095
4c99882
864d5d1
9e10506
9ee6d4d
a1c0562
d880ef7
354c8c4
6b48c6c
d7f82d7
06dd724
cf112e9
4047887
30c899e
4fcaeff
cb6aabc
be7194f
c6170e1
23872cd
6ebe9bb
250ab25
676ae2d
a26fcb4
a4c9d7d
77ebb52
7c3a8f3
19a525c
9a4bce6
6e7575b
29bc1e9
71560b2
82032fd
ed14b31
a27c7c1
4b73b8c
c09d3e6
165d6ff
772ee49
b73eaae
16eb2fd
7bcf640
604164d
f2de60a
2446cc6
c1ec3eb
9c01b9a
67eb575
62b2dea
5d0673e
7b31f0c
20070a0
5937e0d
66fa916
245ded3
16f1285
3f87bec
4175ce8
e67cced
9e48bff
68bd940
3acde54
f3f918c
f0303c4
b30fa76
522d0f6
3a92972
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.
I am going through this sequentially, do we need to derive from these classes? Feels like a code smell.
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 think so. The alternative would be to derive directly from
AuthenticatedGitSourceProvider
and provide overrides for the specific properties/methods (e.g. GitSupportsFetchingCommitBySha1Hash). That feels more likely to drift out of syncNot sure what that means
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.
Personal preference would be to have a class like
TestingStringUtil
in the test project with this method instead of adding it to non testing code if we only need it for testing.Having it here implies it's got production quality tests and some use outside of testing.
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.
On second look we are setting some values on the util here.
This setup feels wrong to me :) It may be out of the scope of the PR but feels like it's structured incorrectly.
Maybe also this class should be split into 2? One for
StringUtil
and one forLocalizationStringUtil
?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 class might be worth revisiting in the future, but it seems out of scope here. With the current design, this seems like the best way to achieve the objective of getting some localization loaded for testing.
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 question about deriving.
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.
Not sure which question this is in reference to
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.
We want to derive here so that we can add/remove plugins without altering any of the rest of the execution
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 question about types. I can't tell from this line the type of
artifact
.