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

Add feed-related commands #88

Merged
merged 5 commits into from Oct 23, 2019

Conversation

dvrylc
Copy link

@dvrylc dvrylc commented Oct 23, 2019

  • Add addfeed and deletefeed commands
  • Implement reactive news feed to support fetching/deleting posts when feeds are added/deleted

@dvrylc dvrylc marked this pull request as ready for review October 23, 2019 07:10
@dvrylc dvrylc requested a review from JunHongT October 23, 2019 07:11
Copy link

@JunHongT JunHongT left a comment

Choose a reason for hiding this comment

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

Thing to take note for further implementation:

  • What will happen if the URL provided is inaccurate?

Comment on lines +72 to 77
private void fetchPosts(ObservableList<FeedPost> feedPostList, List<Feed> feedList) {
Runnable feedPostFetch = () -> {
for (Feed feed : feedList.getFeedList()) {
for (Feed feed : feedList) {
ObservableList<FeedPost> feedPosts = feed.fetchPosts();
Platform.runLater(() -> {
feedPostList.addAll(feedPosts);

Choose a reason for hiding this comment

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

What if the URL provided is not accurate? What will be the response given by the application?

Comment on lines +32 to +33
String name = ParserUtil.parseName(argMultimap.getValue(PREFIX_NAME).get()).fullName;
String address = ParserUtil.parseAddress(argMultimap.getValue(PREFIX_ADDRESS).get()).value;

Choose a reason for hiding this comment

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

Will it be a huge hurdle to build a validation check before adding into the list? Personally believe that if the URL is wrong, then the application should not save it into the list in the first place.

@JunHongT JunHongT merged commit d95f82f into AY1920S1-CS2103T-W11-3:master Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants