search

CS5500-S-2023 / team-lion

team-lion created by GitHub Classroom
0 stars 0 forks source link

Task news drop down #42

Closed gayatribir closed 1 year ago

gayatribir commented 1 year ago

Closes #32 In this implementation, I am extracting news tickers from titles generated from /latestnews command. The AlphaVantage getTickers API is used to confirm the actual tickers and then create drop-down list for those new tickers. Once the list is generated, items are clickable and generate the StringSelect event on NewsCommand class. So, when any item is clicked, it acts as /latestnews command and generates news feeds for that ticker (item which is clicked).

gayatribir commented 1 year ago

@gayatribir Can you add the relevant screenshots as well?

@akuten1298 Added already in the user story. #32

gayatribir commented 1 year ago

@gayatribir Thanks, I checked the screenshots, can you add the screen that gives the latestnews from the ticker clicked from dropdown? The current screenshot says no response for c3.AI

@akuten1298 done. Check out #32 comments.

gayatribir commented 1 year ago

@gayatribir Also can we move out the Javadoc changes for the Price/Quote command out of this PR since this is specific to task news button

@akuten1298 I think these are just comments for the code. I don't think it should break any code in any branch.

gayatribir commented 1 year ago

Yes, the javadoc changes may possibly not break anything but it is considered as the right practice to have a separate PR since it reduces the possibility of conflicts and also keep the focus of the PR on one single change. In case we might want to revert these commits at a later stage or rollback the pr, we would have to rollback the complete set of changes including the javadoc which is not necessary. Since this brings in unnecessary coupling between two separate changes, I would still suggest you to have a separate one.

Yes, the javadoc changes may possibly not break anything but it is considered as the right practice to have a separate PR since it reduces the possibility of conflicts and also keep the focus of the PR on one single change. In case we might want to revert these commits at a later stage or rollback the pr, we would have to rollback the complete set of changes including the javadoc which is not necessary. Since this brings in unnecessary coupling between two separate changes, I would still suggest you to have a separate one.

Sure, I'll raise a different PR. In the meanwhile, Can you review the rest changes?

gayatribir commented 1 year ago

Yes, the javadoc changes may possibly not break anything but it is considered as the right practice to have a separate PR since it reduces the possibility of conflicts and also keep the focus of the PR on one single change. In case we might want to revert these commits at a later stage or rollback the pr, we would have to rollback the complete set of changes including the javadoc which is not necessary. Since this brings in unnecessary coupling between two separate changes, I would still suggest you to have a separate one.

@akuten1298 I have removed javadoc changes for Price Command and raised different PR.

gayatribir commented 1 year ago

Hey guys, I addressed all the feedback on this PR. I would appreciate it if you could review my PR. Thanks! @NiyatiKhandelwal-99 @akuten1298 @abl