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

[RC] Implementation of news functionality from archlinux.org #191

Merged
merged 23 commits into from Jun 3, 2018

Conversation

@steven-omaha
Copy link
Contributor

commented May 30, 2018

No description provided.

steven-omaha added 3 commits May 30, 2018
steven-omaha
fixed formatting
added todos
converted methods to static methods where possible
implemented abstract base methods where necessary
return s.get_data()


def text_wrap(text, max_width=80):

This comment has been minimized.

Copy link
@actionless

actionless May 30, 2018

Owner

isn't it doing the same as pikaur.pprint.format_paragraph()?

steven-omaha
@steven-omaha

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2018

Some of the remaining complaints seem unjustified to me. fd is python convention, pubDate follows the name of the tag and is therefore used as-is with the camel case, and I can't think of why news might be invalid. Is there a way to silence these complaints for these particular lines of code?

@steven-omaha

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2018

@actionless I have some open questions:

  1. From which point in your code should I call the news check?
  2. Should I use your Lock-mechanism for printing the news?
  3. How could I use the pikaur libs to check for the date of the last package install (dat file initilization)?
steven-omaha added 3 commits May 31, 2018
steven-omaha
fixed formatting of news output
replaced news.text_wrap by pprint.format_paragraph
made use of pprint.color_line
steven-omaha
@actionless

This comment has been minimized.

Copy link
Owner

commented May 31, 2018

  1. i think after refreshing package db is good time: https://github.com/actionless/pikaur/blob/master/pikaur/main.py#L113
  2. right now that shouldn't be needed since at the time of showing arch news no other thread will be running, however just in case of some feature changes you can just use pikaur.pprint.print_stdout which behaves like print() but with lock
python -c "from pikaur.pacman import PackageDB ; print(sorted([p for p in PackageDB.get_local_list() if p.name in [r.name for r in PackageDB.get_repo_list()]], key=lambda x: -x.installdate)[0].installdate)"
@actionless

This comment has been minimized.

Copy link
Owner

commented May 31, 2018

i think you can beautify and make third thing as a new method of PackageDB instead of having that code inside news module

@actionless

This comment has been minimized.

Copy link
Owner

commented May 31, 2018

fd is python convention,

use last_date_fd or so

pubDate follows the name of the tag and is therefore used as-is with the camel case

i think it's just easier to rename it to pub_date instead of writing directive to disable a warning

and I can't think of why news might be invalid.

because it's actually a "constant"/"global" by pylint convention, you could rewrite it like this to avoid defining it:

if __name__ == '__main__':
    News().check_news()
last_seen_news_date = datetime.datetime.strptime(
self._last_seen_news, '%a, %d %b %Y %H:%M:%S %z'
)
if len(last_online_news) == 0:

This comment has been minimized.

Copy link
@actionless

actionless May 31, 2018

Owner

if not last_online_news:

This comment has been minimized.

Copy link
@steven-omaha

steven-omaha May 31, 2018

Author Contributor

Isn't the check by len() more intuitive, because last_online_news is a string? Your check assumes implicit conversion to bool.

This comment has been minimized.

Copy link
@steven-omaha

steven-omaha May 31, 2018

Author Contributor

Nevermind, PEP8 says you're right. 65ba958

steven-omaha
added type hinting
more bugfixes for Travis checks
changed print() to pprint.print_stdout()
self._last_seen_news = self._get_last_seen_news()

def check_news(self) -> None:
rss_feed: str = self._get_rss_feed()

This comment has been minimized.

Copy link
@actionless

actionless May 31, 2018

Owner

you could instead define def _get_rss_feed(self) -> str:

This comment has been minimized.

Copy link
@steven-omaha

steven-omaha May 31, 2018

Author Contributor

Fixed. 5785996 and 994559f

steven-omaha added 2 commits May 31, 2018
@actionless

This comment has been minimized.

Copy link
Owner

commented May 31, 2018

right now that shouldn't be needed since at the time of showing arch news no other thread will be running,

i've just realized what it could be really nice to do package db update and news update in two different threads, and after print the news, for making that possible i recommend to split check_news() into twoparts, one for retrieving latest news (mb call it from __init__) and one for printing the news, so it will be looking like that (pseudo-code):

news = News()
with ThreadPool() as p:
  p.apply(refresh_package_db)
  p.apply(news.get_latest)
news.print_latest()

i can write the multithreading part on my own after merging this

however print lock still not needed for that :-)

from typing import TextIO

from pikaur.config import CACHE_ROOT
import pikaur.pprint

This comment has been minimized.

Copy link
@actionless

actionless May 31, 2018

Owner

from pikaur.pprint import print_stdout, format_paragraph

This comment has been minimized.

Copy link
@steven-omaha

steven-omaha May 31, 2018

Author Contributor

Done. 3407edd

This comment has been minimized.

Copy link
@actionless

actionless May 31, 2018

Owner

oops, from .pprint import....

This comment has been minimized.

Copy link
@actionless

actionless May 31, 2018

Owner

btw you can run the check locally, ./maintenance_scripts/lint.sh

This comment has been minimized.

Copy link
@steven-omaha

steven-omaha May 31, 2018

Author Contributor

The imports will be fixed in the last step, have some issues with my IDE.

@steven-omaha

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2018

I agree on the multithreading part. I already use pprint.print_stdout().

@steven-omaha

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2018

I seem to cannot answer on your comments. Your proposed news fetch on __init__ and then printing from a public method is done in 6942923. I'm out, enough coding for today. Expect more after the weekend.

@actionless

This comment has been minimized.

Copy link
Owner

commented May 31, 2018

that's already quite an impressive work

don't hesitate to ask if you'll have problems with multithreading part, but it should be easy, you can see here as in example, but for this case it should be even simpler:

https://github.com/actionless/pikaur/blob/master/pikaur/main.py#L136-L151

@steven-omaha

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2018

All necessary functions are implemented. If you pikaur -Su, you should see news if there are some.

Missing:

  1. translations (I won't do this, got no experience with that)
  2. fetching news in separate thread, printing them after fetching all DB updates (EDIT: @actionless see 017496d and tell me if this is ok, I never used threading for this kind of stuff)
steven-omaha
@steven-omaha

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2018

If there a no further objections, you are free to pull this.

@steven-omaha steven-omaha changed the title [WIP] Implementation of news functionality from archlinux.org [RC] Implementation of news functionality from archlinux.org Jun 1, 2018

steven-omaha
More code cleanup
Renamed get_latest() to fetch_latest(), to avoid confusions with getters
removed unnecessary _get_rss_feed() and merged it into fetch_news()
@actionless

This comment has been minimized.

Copy link
Owner

commented Jun 3, 2018

thanks again for your great work!

@actionless actionless merged commit e3f9bce into actionless:master Jun 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@steven-omaha steven-omaha deleted the steven-omaha:news branch Jun 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.