Skip to content

Conversation

@virkt25
Copy link
Contributor

@virkt25 virkt25 commented Mar 7, 2018

I'd like to get opinions on this PR but in reviewing this page, it provides no relevant information to a user. I think this page should be deleted as such and added at a time when this feature has been implemented. Having a TBD page on features not yet implemented doesn't provide much value as a user can see our roadmap via our Milestones in LoopBack 4 Repo.

We already have issues to capture adding such features in loopback-next monorepo.

@virkt25 virkt25 requested a review from bschrammIBM as a code owner March 7, 2018 02:37
@virkt25 virkt25 self-assigned this Mar 7, 2018
@virkt25 virkt25 requested review from a team, b-admike, dhmlau, jannyHou, kjdelisle, raymondfeng and shimks and removed request for a team March 7, 2018 02:37
@virkt25 virkt25 force-pushed the remove/calling-apis branch from 9af7d71 to 1e18ab0 Compare March 7, 2018 15:22
@shimks
Copy link
Contributor

shimks commented Mar 7, 2018

I've noticed that other parts of the lb4 docs are also littered with subheaders on calling other APIs with note on them underneath saying it's TBD (example being Testing-your-application.md). If we were to agree on this, could you also get rid of those references throughout the docs/lb4 folder in this PR?

@dhmlau
Copy link
Member

dhmlau commented Mar 7, 2018

I'm good with either way:

  • Removing it due to the reasons stated in the description of the PR
  • Keeping it as we are continuously updating the docs, so there is no need to have a good snapshot of the docs and call it as Dev Preview Add prism syntax highlighting #2.

@virkt25
Copy link
Contributor Author

virkt25 commented Mar 8, 2018

@shimks I don't see any links in Testing your application to this page. I did check to see if there were any links to this page and didn't find any. I do see that there are some section in that page which are TBD and I'm ok with those being there since it's not the entire doc that is a TBD.

Can you point me to a line if I'm not seeing it please.

@shimks
Copy link
Contributor

shimks commented Mar 9, 2018

My bad, I guess I was being hyperbolic when I said 'littered with'; I could only find incomplete references of calling other API (services) in Testing-yourapplication.md (by searching for the word service in the repo). I'll just get rid of this as part of my work in #635.

Copy link
Contributor

@shimks shimks left a comment

Choose a reason for hiding this comment

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

IMO, we shouldn't delete the page. I'd rather rename it to something like Calling-other-APIs-and-Web-Services.SHELVED and then remove the reference of it from the sidebar. This would allow us to keep a track of the fact that we plan on (or at least planned on) adding docs for this section, and I think we can delegate the discussion of removal (or revival) of this page in a planning session for GA or whatever when it crops up.

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

IMO if we want to implement new rest or soap feature, or refactor the ones in LB3, we can open issues to discuss it, a placeholder for document does not quite help move the progress...

Copy link
Contributor

@bschrammIBM bschrammIBM left a comment

Choose a reason for hiding this comment

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

approved

@virkt25
Copy link
Contributor Author

virkt25 commented Mar 11, 2018

I'd rather rename it to something like Calling-other-APIs-and-Web-Services.SHELVED and then remove the reference of it from the sidebar.

I'd agree with that for pages with somewhat meaningful content on them. This page is all just a big TBD so I think it's better to delete this one.

cc: @raymondfeng thoughts?

@dhmlau
Copy link
Member

dhmlau commented Mar 11, 2018

I'd rather keep it or toss it, instead of renaming it to something else. :) Few months later, we might forget about this renamed page.

@raymondfeng
Copy link
Contributor

I'm with @dhmlau. The feature is coming - loopbackio/loopback-next#1119.

If we think the pages are confusing, we can add some disclaimer there to make it clear.

@virkt25
Copy link
Contributor Author

virkt25 commented Mar 12, 2018

@raymondfeng there is no confusing as there is a Content TBD disclaimer on the page. I just think it's better for documentation to reflect the current state of the project (basically it doesn't exist right now). The PR is a POC, so while the feature is coming, it may still be a while. The Docs should be done as a part of that PR explaining Usage, etc.

I'm not opposed to keeping this page but think it's better to reflect the current state. If you disagree I can close this PR, and mark the review for this page as done. :)

@virkt25
Copy link
Contributor Author

virkt25 commented Mar 22, 2018

closing in favour of loopbackio/loopback-next#1177

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.

7 participants