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

Proposed change to fix Issue 491. #493

Merged
merged 3 commits into from Jul 31, 2013

Conversation

Projects
None yet
4 participants
@martincostello
Contributor

martincostello commented Jul 29, 2013

Proposed change to fix Issue 491. The wording isn't neccessarily exactly what the message should say, but it's a suggestion. Other than rewording the message, I don't think any other changes would be needed.

I've avoided having the unit test assert on the exact URL of the message.

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Jul 29, 2013

Collaborator

Hi @martincostello thanks for the proposed fix.

I'm not sure if we should include the additional information as proposed, since there are more reasons and possible configurations that makes that we can't locate the closing body tag.

But I agree that we could add some more information as where to look for if this message appears in the log file.
So I wonder if it would be OK for you if we only add a link to our troubleshooting page and that the extra information you supplied in this PR is added to our troubleshooting wiki page (wiki changes will be published on the getglimpse, but not automatically)

We are reworking the documentation, so the page reference might change in the future to a more dedicated page where we list all possible issues (known up until then) that might trigger that log message.

What do you think?

Collaborator

CGijbels commented Jul 29, 2013

Hi @martincostello thanks for the proposed fix.

I'm not sure if we should include the additional information as proposed, since there are more reasons and possible configurations that makes that we can't locate the closing body tag.

But I agree that we could add some more information as where to look for if this message appears in the log file.
So I wonder if it would be OK for you if we only add a link to our troubleshooting page and that the extra information you supplied in this PR is added to our troubleshooting wiki page (wiki changes will be published on the getglimpse, but not automatically)

We are reworking the documentation, so the page reference might change in the future to a more dedicated page where we list all possible issues (known up until then) that might trigger that log message.

What do you think?

@martincostello

This comment has been minimized.

Show comment
Hide comment
@martincostello

martincostello Jul 29, 2013

Contributor

Yeah sure, I don't see a problem with that approach, it's probably less brittle. I think a link from the Glimpse log to the Wiki page would be an improvement on the current log content (I'm going to add it with some content for my specific fix now).

Would you like me to make that change on my fork and re-submit the change, or will you handle that yourself?

Contributor

martincostello commented Jul 29, 2013

Yeah sure, I don't see a problem with that approach, it's probably less brittle. I think a link from the Glimpse log to the Wiki page would be an improvement on the current log content (I'm going to add it with some content for my specific fix now).

Would you like me to make that change on my fork and re-submit the change, or will you handle that yourself?

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Jul 29, 2013

Collaborator

hey @martincostello you can make the code change in your fork and we'll pull it in afterwards.

The only thing we need in addition is for you to fill out the contributor agreement and have it mailed to anthony dot vanderhoorn at where the google mail is, so that we can officially accept your first contribution to this project. /cc: @avanderhoorn @nikmd23

Collaborator

CGijbels commented Jul 29, 2013

hey @martincostello you can make the code change in your fork and we'll pull it in afterwards.

The only thing we need in addition is for you to fill out the contributor agreement and have it mailed to anthony dot vanderhoorn at where the google mail is, so that we can officially accept your first contribution to this project. /cc: @avanderhoorn @nikmd23

@martincostello

This comment has been minimized.

Show comment
Hide comment
@martincostello

martincostello Jul 29, 2013

Contributor

Will send the CLA over tomorrow. I've assumed you wanted the link to be to getglimpse.com (for when the Wiki changes are pulled in later), rather than the Wiki on Github. Will update if that's incorrect.

Contributor

martincostello commented Jul 29, 2013

Will send the CLA over tomorrow. I've assumed you wanted the link to be to getglimpse.com (for when the Wiki changes are pulled in later), rather than the Wiki on Github. Will update if that's incorrect.

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Jul 29, 2013

Collaborator

@martincostello yep, link towards getglimpse.com but the content of that page can be adapted through the wiki

PR looks good (although if you could fix the typo in the constant name, that would be awesome ;-))

Collaborator

CGijbels commented Jul 29, 2013

@martincostello yep, link towards getglimpse.com but the content of that page can be adapted through the wiki

PR looks good (although if you could fix the typo in the constant name, that would be awesome ;-))

@martincostello

This comment has been minimized.

Show comment
Hide comment
@martincostello

martincostello Jul 30, 2013

Contributor

CLA emailed as requested.

Contributor

martincostello commented Jul 30, 2013

CLA emailed as requested.

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Jul 31, 2013

Member

Good PR! I've wanted this message to be updated for a while now. This fixes #491

Member

avanderhoorn commented Jul 31, 2013

Good PR! I've wanted this message to be updated for a while now. This fixes #491

avanderhoorn added a commit that referenced this pull request Jul 31, 2013

Merge pull request #493 from martincostello/master
Proposed change to fix Issue 491.

@avanderhoorn avanderhoorn merged commit 671c6c6 into Glimpse:master Jul 31, 2013

1 check passed

default TeamCity Build Glimpse :: Continuous Integration finished: Tests passed: 949, ignored: 10
Details
@nikmd23

This comment has been minimized.

Show comment
Hide comment
@nikmd23
Member

nikmd23 commented Aug 2, 2013

Thanks @martincostello!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment