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

fix typo #194

Closed
wants to merge 24 commits into from

Conversation

@KangZhiDong KangZhiDong reopened this Aug 23, 2019
KangZhiDong added 10 commits Aug 24, 2019
remove unuse method
fix typo
fix typo
…asper.servlet.TldScanner.webxmlSkip、webxmlFailPathDoesNotExist、webxmlSkip” are joined by other String
@KangZhiDong

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2019

@markt-asf fix some typos at the weekend (●´∀`●)ノ

KangZhiDong added 10 commits Aug 31, 2019
Fix typo
fix typo
@KangZhiDong

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

@markt-asf please review

@xiantang

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

@KangZhiDong too much commits

KangZhiDong added 2 commits Sep 11, 2019
Fix typo
remove extra ‘a’
Fix typo
add missing javadoc
@markt-asf

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

Wow. There are a lot of changes here. Thank you.

It is too much to review in one go. I took a quick look at the first few changes files and things look good. My current plan is to review these in detail commit by commit and cherry-pick each commit once it has been reviewed.

I appreciate that some of these commits may depend on previous commits. That said, more PRs with fewer commits would be easier to work with where that is possible.

@markt-asf

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

I'm about 2/3 of the way through the commits. With the benefit of experience, 1 PR vs multiple PRs doesn't make that much of a difference. What would have helped was re-basing the PR against master. I've done this locally and there were only a couple of conflicts and it makes it much easier to review.

I spotted a few unused i18n strings that weren't removed from the translations. That isn't a big deal as the process we use to import/export to/from POEditor will clean those up.

I have spotted a couple of minor issues. I'll fix those before merging the commits. I'll also comment on the commit in this PR where it makes sense to do so.

@@ -101,7 +101,7 @@ policy had to be relaxed. This is not recommended for production environments.
&lt;service class&gt;/&lt;host&gt;:&lt;port&gt;/&lt;service name&gt;</code>.
The SPN used in this how-to is <code>HTTP/win-tc01.dev.local</code>. To
map the user to the SPN, run the following:
<source>setspn -A HTTP/win-tc01.dev.local tc01</source>
<source>setspn -An HTTP/win-tc01.dev.local tc01</source>
</li>

This comment has been minimized.

Copy link
@markt-asf

markt-asf Sep 11, 2019

Contributor

This change is incorrect. This is a command line option hence it needs to be .. -A HTTP...

This comment has been minimized.

Copy link
@xiantang

xiantang Sep 11, 2019

Contributor

I'm about 2/3 of the way through the commits. With the benefit of experience, 1 PR vs multiple PRs doesn't make that much of a difference. What would have helped was re-basing the PR against master. I've done this locally and there were only a couple of conflicts and it makes it much easier to review.

I spotted a few unused i18n strings that weren't removed from the translations. That isn't a big deal as the process we use to import/export to/from POEditor will clean those up.

I have spotted a couple of minor issues. I'll fix those before merging the commits. I'll also comment on the commit in this PR where it makes sense to do so.

lol ! It is so hard to review

@KangZhiDong

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

It seems to have been merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.