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.

Sign up for GitHub

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

Jump to bottom

Break out readInputStream from Main.java to RespInputParser.java #99 #139

Merged
merged 6 commits into from
Feb 6, 2024
Merged

Break out readInputStream from Main.java to RespInputParser.java #99 #139

merged 6 commits into from
Feb 6, 2024

Conversation

RobertMili
Copy link
Contributor

  1. Adding test for that class

@RobertMili
1. Adding the RespInputParser class
4643688 
2. Adding test for that class
@RobertMili RobertMili linked an issue Feb 16, 2023 that may be closed by this pull request
Break out readInputStream from Main.java to RespInputParser.java #99
Closed
@RobertMili RobertMili changed the title 1. Adding the RespInputParser class Break out readInputStream from Main.java to RespInputParser.java #99 Feb 16, 2023
@RobertMili
1. Adding the RespInputParser class
62e1436 
2. Adding test for that class
3. Adding more test on MainTest.java
@RobertMili
Removing unnecessary code
4a8d7cf
@RobertMili RobertMili mentioned this pull request Feb 16, 2023
Closed
@RobertMili RobertMili force-pushed the 99-break-out-readinputstream-from-mainjava-to-respinputparserjava branch from 09b792b to e3c700a Compare February 17, 2023 19:28
@sonarcloud
Copy link

sonarcloud bot commented Feb 17, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

robinalfengard
robinalfengard approved these changes Feb 6, 2024
View reviewed changes
Copy link
Contributor

@robinalfengard robinalfengard left a comment

Choose a reason for hiding this comment

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

Results in a much cleaner main. Also good to include separate tests for the respInputParser

@kappsegla
Copy link
Contributor

Results in a much cleaner main. Also good to include separate tests for the respInputParser

When trying the code locally, have you merged it with latest version of main to make sure everything works after merge? Or are there no conflicts against main branch?

cmatlak
cmatlak approved these changes Feb 6, 2024
View reviewed changes
Copy link
Contributor

@cmatlak cmatlak left a comment

Choose a reason for hiding this comment

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

good thinking breaking it out from main, and great with the added tests

@kappsegla
Copy link
Contributor

Looks like we have a problem with some of the commits not signed 😔. So we have two options override the branch protection or redo the commits with signing and maybe adding the original author as co-author?

@cmatlak
Copy link
Contributor

cmatlak commented Feb 6, 2024

What is the preferred method to solve this?

@robinalfengard
Copy link
Contributor

Results in a much cleaner main. Also good to include separate tests for the respInputParser

When trying the code locally, have you merged it with latest version of main to make sure everything works after merge? Or are there no conflicts against main branch?

I tested the merge with latest version of main and I managed to build the program successfully!

@kappsegla kappsegla merged commit bf4a184 into main Feb 6, 2024
@kappsegla kappsegla deleted the 99-break-out-readinputstream-from-mainjava-to-respinputparserjava branch February 6, 2024 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmatlak cmatlak cmatlak approved these changes

@robinalfengard robinalfengard robinalfengard approved these changes

@jLereback jLereback Awaiting requested review from jLereback

@safstromo safstromo Awaiting requested review from safstromo

@kappsegla kappsegla Awaiting requested review from kappsegla

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Break out readInputStream from Main.java to RespInputParser.java
4 participants
@RobertMili @kappsegla @cmatlak @robinalfengard