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

Fix candidates and PA #2

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix candidates and PA #2

wants to merge 3 commits into from

Conversation

thierryvolpiatto
Copy link
Member

This fix PA and simplify code.

Thierry Volpiatto added 3 commits March 15, 2019 12:33
Add headers and simplify code.
which should be implemented as dummy source
@alphapapa
Copy link
Member

Hi Thierry,

  1. The persistent action worked fine for me before this PR. Maybe my Helm version is too old?

  2. I think this PR makes the summaries less useful, as most of them now are only one or two sentences. The code before this PR uses the Wikipedia "extracts" API, which I set up in the Wikipedia API sandbox, and AFAICT it's intended for the purpose of summarizing pages. It usually provides a paragraph or two, which easily fits on the screen, and provides enough information that, most of the time, I don't need to visit the full article. This PR seems to make the summaries more like dictionary definitions, which are too brief, IMO.

What do you think?

Thanks.

@thierryvolpiatto
Copy link
Member Author

thierryvolpiatto commented Mar 16, 2019 via email

@alphapapa
Copy link
Member

BTW I have fixed the re-search-forward call to avoid error

I actually left it that way on purpose, so it would raise an error if the HTTP request can't be parsed correctly rather than failing silently. I'd rather users report a bug than think there are no results available or that the tool simply doesn't work sometimes. Ideally we wouldn't have to skip the HTTP headers manually, but Emacs network libraries are so primitive. :( Anyway, what do you think?

and improved header as well, feel free to add your copyright if you want.

I've only contributed a few lines, so I think it's fine as is. :)

Now working fine for me with and without curl usage.

Good. So does this PR still require merging?

Thanks.

@thierryvolpiatto
Copy link
Member Author

thierryvolpiatto commented Mar 18, 2019 via email

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.

2 participants