This template rewrite brings the following things to the table.
Do take a look and let me know if there are anymore changes needed in here.
Thanks.
Should this be translatable content ? Currently missing those tags.
Hey @copperi, the current rendition of easyfix does not have them so I am not really sure if this should have this or not. (Maybe it should? You would want to ask around for more details on this.)
Thanks for the review.
I dislike the habit that pinpoints the dependencies to a very very specific version. Please enforce a minimal version if needed but let's not specify the higher version, unless we know that a newer version breaks the app.
we can drop fedmenu, it's being deprecated
A lot of the dependencies listed here don't seem to be used by the project: certifi, chardet, idna, requests, requests-oauthlib, oauthlib, urllib3, six
Could you check it?
Yep, I ran a pip3 freeze > requirements.txt cold. Let me remove the version peculiarities in it.
pip3 freeze > requirements.txt
Alright. Will remove it.
A lot of the dependencies listed here don't seem to be used by the project: certifi, chardet, idna, requests, requests-oauthlib, oauthlib, urllib3, six Could you check it?
Okay, I will reflect the changes in the newer requirements.txt.
requirements.txt
So,
python-bugzilla
requests
certifi
chardet
idna
urllib3
mwclient
six
requests-oauthlib
oauthlib
There were no differences possible in the requirements.txt hence in terms of dependency listing but I provided for given version or above check for dependencies.
Re-requesting review @pingou.
python-bugzilla uses requests and requests in turn uses certifi, chardet, idna and urllib3.
So the first one is required in the file, the others not. Imagine python-bugzilla drops requests, or requests drops certifi, if you keep it in the requirements.txt you will still bring it while it's not needed.
mwclient uses six and requests-oauthlib and requests-oauthlib requires idna, certifi, urllib3, chardet and oauthlib.
Same as above.
You put your requirements in that file, not the dependencies of your dependencies, unless there is something optional or a bug in the dependency chain higher up.
Understood. I'd change them right away.
Amended 6219dcf https://pagure.io/fork/t0xic0der/fedora-gather-easyfix/c/6219dcfe3e5f89e2da65c0688ecac45b5e2b5dc9 to include only the necessary dependencies.
@pingou The suggested changes have been made and this PR is ready to be reviewed again. Please do take a look whenever you get time.
Had just a quick glance, is there any reason for renaming the template?
Also, do we want to review the git commit history?
Not really. Should I fall back to the original name?
One less change, always good :)
Alright. I will revert the renaming.
Is there anything else to correct/change so that I can do them all in a go?
Of the easyfix repo? or of those projects listed there?
easyfix
Also, do we want to review the git commit history? Of the easyfix repo? or of those projects listed there?
Of the PR here
I quickly sifted through the commit history and found that nearly all the histories have meaningful commit messages and there are notable differences between subsequent commits that there would not be any need of rewriting the history (if that's what you're asking).
@pingou, I have made some more changes. I have reverted back to template.html name and made corrections to setup.py regarding the static file inclusion. Should we bump its version when the modernization is complete?
template.html
setup.py
Also, I have included the logo provided by @duffy here https://pagure.io/design/issue/714#comment-717787 and have accounted for the suggestions made by @duffy and @mleonova about the look and feel here https://pagure.io/design/issue/715#comment-717513 and here https://pagure.io/design/issue/715#comment-716852.
The modernization would be further discussed about by the design team as a heuristic evaluation of the progress made so far was done. There's more about it here https://pagure.io/design/issue/715#comment-718574.
Re-requesting review.
Should we include the previous people involved? Or maybe just drop that line?
rebased onto 66a2c839dc024f8a9af3ff38952558b417bc51bd
To https://pagure.io/fedora-gather-easyfix/pull-request/9#comment-146146, that was automatically generated by my IDE but I believe that we would want to add the names of the previous people involved there instead of removing that line.
The design is worked upon here in this ticket https://pagure.io/design/issue/715 so I would make changes once the mockups have been decided upon.
Dropping it sounds easier to me but it's up to you
This template rewrite brings the following things to the table.
Do take a look and let me know if there are anymore changes needed in here.
Thanks.