liaochong / myexcel

MyExcel, a new way to operate excel!
https://github.com/liaochong/myexcel/wiki
Apache License 2.0
1.66k stars 325 forks source link

I have applied Refactoring techniques on the project as I have got assignment from my university kindly look into my changes and accept the request in your feature branch #356

Closed jenishp619 closed 2 years ago

jenishp619 commented 2 years ago

请提交至 pre-release 分支,勿直接提交至master分支。

请包含更改摘要以及修复的问题。还请包括相关的动机和背景。列出此更改所需的所有依赖项。

Please submit to the pre-release branch, do not submit directly to the master branch.

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

liaochong commented 2 years ago

Thank you for submitting pull requests, but I'm sorry that the content you submitted does not meet the actual needs, so this submission will be abandoned. In myexcel version 4.0.1, there is a bug in the SaxExcelReader: in the doReadXls method, if the detectedMerge = true, but the mergecellIndexMapping is found to be empty after parsing, the detectedMerge should be set to false, otherwise a null pointer exception will be thrown. Can you help fix it?

jenishp619 commented 2 years ago

okay i will look into it but will you accept the pull request in the feature branch that I have already submitted? https://github.com/liaochong/myexcel/pull/357 This is working well

On Mon, Mar 21, 2022 at 4:44 AM 清沐 @.***> wrote:

Thank you for submitting pull requests, but I'm sorry that the content you submitted does not meet the actual needs, so this submission will be abandoned. In myexcel version 4.0.1, there is a bug in the SaxExcelReader: in the doReadXls method, if the detectedMerge = true, but the mergecellIndexMapping is found to be empty after parsing, the detectedMerge should be set to false, otherwise a null pointer exception will be thrown. Can you help fix it?

— Reply to this email directly, view it on GitHub https://github.com/liaochong/myexcel/pull/356#issuecomment-1073562286, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMXD7FJEZRIH3B6XFDFRM5DVBASFPANCNFSM5RE3NKMQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

liaochong commented 2 years ago

These two submissions are the same, so they won't be adopted. First look at the bug I said. It's very simple. I think you can fix it quickly, so I can merge it as soon as possible

jenishp619 commented 2 years ago

So in order to accept my changes what should I do? Can you tell me what changes should be made? And can you consider my changes for refactoring I want bonus points for assignment so I am requesting you to guide me

On Mon, Mar 21, 2022, 7:10 AM Jenish Patel @.***> wrote:

okay i will look into it but will you accept the pull request in the feature branch that I have already submitted? https://github.com/liaochong/myexcel/pull/357 This is working well

On Mon, Mar 21, 2022 at 4:44 AM 清沐 @.***> wrote:

Thank you for submitting pull requests, but I'm sorry that the content you submitted does not meet the actual needs, so this submission will be abandoned. In myexcel version 4.0.1, there is a bug in the SaxExcelReader: in the doReadXls method, if the detectedMerge = true, but the mergecellIndexMapping is found to be empty after parsing, the detectedMerge should be set to false, otherwise a null pointer exception will be thrown. Can you help fix it?

— Reply to this email directly, view it on GitHub https://github.com/liaochong/myexcel/pull/356#issuecomment-1073562286, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMXD7FJEZRIH3B6XFDFRM5DVBASFPANCNFSM5RE3NKMQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

jenishp619 commented 2 years ago

Ok, will do that. So after that fix will you do my work for refactoring

On Mon, Mar 21, 2022, 7:25 AM 清沐 @.***> wrote:

These two submissions are the same, so they won't be adopted. First look at the bug I said. It's very simple. I think you can fix it quickly, so I can merge it as soon as possible

— Reply to this email directly, view it on GitHub https://github.com/liaochong/myexcel/pull/356#issuecomment-1073727675, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMXD7FJ7LOTD5G72PHYVDWDVBBFAVANCNFSM5RE3NKMQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

liaochong commented 2 years ago

Isn't fixing bugs part of refactoring?

jenishp619 commented 2 years ago

No, it doesn't. my assignment is to refactor the code in terms of readability and duplication of code

On Mon, Mar 21, 2022 at 7:27 AM 清沐 @.***> wrote:

Isn't fixing bugs part of refactoring?

— Reply to this email directly, view it on GitHub https://github.com/liaochong/myexcel/pull/356#issuecomment-1073729663, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMXD7FKQD3OWP6NGZMCGO4DVBBFKDANCNFSM5RE3NKMQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

jenishp619 commented 2 years ago

on which branch should i send you pull request ?

On Mon, Mar 21, 2022 at 7:28 AM Jenish Patel @.***> wrote:

No, it doesn't. my assignment is to refactor the code in terms of readability and duplication of code

On Mon, Mar 21, 2022 at 7:27 AM 清沐 @.***> wrote:

Isn't fixing bugs part of refactoring?

— Reply to this email directly, view it on GitHub https://github.com/liaochong/myexcel/pull/356#issuecomment-1073729663, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMXD7FKQD3OWP6NGZMCGO4DVBBFKDANCNFSM5RE3NKMQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

liaochong commented 2 years ago

Hotfix/4.0.2.

Well, there are many things that need to be refactored, but this needs to be discussed. After you find the code that needs to be refactored, submit a small part or email to me to discuss whether it is necessary to refactor, so as to ensure that your work is effective, otherwise it may waste your time.

jenishp619 commented 2 years ago

can you tell me exactly what file needs to be fixed and your problem again?

On Mon, Mar 21, 2022 at 7:35 AM 清沐 @.***> wrote:

Hotfix/4.0.2 https://github.com/liaochong/myexcel/tree/Hotfix/4.0.2.

Well, there are many things that need to be refactored, but this needs to be discussed. After you find the code that needs to be refactored, submit a small part or email to me to discuss whether it is necessary to refactor, so as to ensure that your work is effective, otherwise it may waste your time.

— Reply to this email directly, view it on GitHub https://github.com/liaochong/myexcel/pull/356#issuecomment-1073735508, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMXD7FI2T2XPGBXVQYEFV4LVBBGFPANCNFSM5RE3NKMQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>