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

QnAMakerService correctly builds hostname URL & throws error w/o URL #1035

Merged
merged 2 commits into from
Jul 3, 2019

Conversation

Zerryth
Copy link
Contributor

@Zerryth Zerryth commented Jul 3, 2019

Fixes #939

Description

  • Issue 1: URL built for hostname would always append "/qnamaker" to QnAMakerService.hostname, even when the hostname already had /qnamaker in the hostname string

    • QnAMakerService will now only append /qnamaker to hostname, if it does not end in /qnamaker already
    • Ex 1: https://myservice.azurewebsites.net => https://myservice.azurewebsites.net/qnamaker
    • Ex 2: https://MyServiceThatDoesntNeedAppending.azurewebsites.net/qnamaker remains the same
  • Issue 2: previously if QnAMakerService's first parameter receives a source that does not have a hostname, in the URL class's logic, it would throw invalid URL error when constructing URL + base to create QnAMakerService.hostname

    • As per Steven's advice, QnAMakerService now throws an error, clearly stating that the hostname is required in the source to construct the QnAMakerService properly
    • this is because nothing actually can be done w/regards to QnAMaker unless you have a hostname

@Zerryth Zerryth requested a review from stevengum July 3, 2019 21:30

this.hostname = new URL('/qnamaker', this.hostname).href;
if (!this.hostname.endsWith('/qnamaker')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm being picky, "/qnamaker" isn't a host name.

"/qnamaker" is a path segment.

In this case, I think the underlying IQnAService is messed up, so we can't change it.

@cleemullins cleemullins merged commit c4998e1 into master Jul 3, 2019
@cleemullins cleemullins deleted the Zerryth/QnAMakerServiceHostname branch July 3, 2019 23:01
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.

QnaMakerService constructor throws
2 participants