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.

Sign up for GitHub

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

Jump to bottom

Fix warnings headers #73

Merged
merged 1 commit into from Jan 5, 2021
Merged

Fix warnings headers #73

merged 1 commit into from Jan 5, 2021

Conversation

jkfran
Copy link
Contributor

@jkfran jkfran commented Jan 4, 2021 •
edited

Done

  • Update warning header name from warning to discourse-warning.
  • Removed '199 canonicalwebteam.discourse ' at the beginning of each header value
  • Remove false possitive warnings and improve warnings messages for:
    • Missing topic link for a path
    • Missing path for a topic link

QA

Follow the QA instruction here canonical/dqlite.io#100

@jkfran jkfran force-pushed the parser-fix-warnings-headers branch from 48893aa to 0d238ca Compare January 4, 2021 17:30
@carkod
Copy link
Contributor

carkod commented Jan 4, 2021

Are we still using the CHANGES.txt?

@jkfran
Copy link
Contributor Author

jkfran commented Jan 4, 2021

Are we still using the CHANGES.txt?

Good point, all the changes are now tracked here: https://github.com/canonical-web-and-design/canonicalwebteam.discourse/releases

I think is better than maintaining the file because now it's done automatically. I think we can remove it.

nottrobin
nottrobin approved these changes Jan 5, 2021
View reviewed changes
Copy link
Contributor

@nottrobin nottrobin left a comment

Choose a reason for hiding this comment

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

LGTM 👍

But am I right in thinking that this doesn't fix the issue that warnings persist through the whole of the application lifetime? We should also fix that ASAP.

@jkfran
Copy link
Contributor Author

jkfran commented Jan 5, 2021

LGTM +1

But am I right in thinking that this doesn't fix the issue that warnings persist through the whole of the application lifetime? We should also fix that ASAP.

It does, _set_parser_warnings will set as headers the last 10 parser warnings and right after reset the parser object warnings. We will still have one instance of the parser but the warnings will be persistent just for one request (one request to the actual docs site not from our backend to discourse)

@jkfran jkfran merged commit 2b796e9 into canonical:master Jan 5, 2021
jkfran added a commit that referenced this pull request Jan 5, 2021
@jkfran
Delete CHANGES.txt
b2f45d0 
Are we still using the `CHANGES.txt`?

_Originally posted by @carkod in #73 (comment)

The `CHANGES.txt` is no longer needed for this module in my opinion. We can track now the releases on GitHub for this repository:
https://github.com/canonical-web-and-design/canonicalwebteam.discourse/releases

These GitHub releases are created automatically whenever we update the package version from a PR.
@jkfran jkfran mentioned this pull request Jan 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nottrobin nottrobin nottrobin approved these changes

Assignees
No one assigned
Labels
Review: Code +1 Review: QA +1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jkfran @carkod @nottrobin