On Thu, Mar 21, 2019 at 8:35 PM Umesh Singla <umeshksin...@macports.org> wrote:
> a) We have seen a quick demo of this already. However, the major part I > think is missing is the search. We can brainstorm over the details like > search-as-you-type, adding new ports etc according to the timeline. Not > sure how much browsing by the first letter helps. > Search-as-you-type would be good, and can be further supported with: "New Ports", "Related Ports" on port detail, "Popular Ports"- overall and for each category. > b) You cannot have a class Ports when it represents a Port: > https://github.com/arjunsalyan/MacPorts-Demo-App/blob/master/ports/models.py#L6 > . > Yes, there are some major code improvements to be done. I will finish these shortly. > 2. build statistics: > > a) In Time Elapsed of builds, it would be incorrect to show time taken for > only one of the build stages. Example, in the case of > https://build.macports.org/builders/ports-10.12_x86_64-builder/builds/87301, > total-time-taken (12 min 59 secs) is right to be shown and not "6 mins 45 > secs". > Thanks for picking this out. > b) There are some things which seem hard-coded to me. I > see '10.14_x86_64', '10.13_x86_64' at multiple places - in port detail > view, build to database view and jinja templates. It's time to define some > constants config file now. For build statuses as well. With a new release > of macOS, we do not want to have to change multiple files in code. In this > project, it is important that a part which works, it is accurate and > complete. > What I have planned is to have a separate table of builders with relations to the build history table. Any upcoming versions can then easily be added to the table. Since, I wasn't fetching many logs from c) Also, as Mojca mentioned, errors like these: > http://frozen-falls-98471.herokuapp.com/ports/database/ should not be > exposed. What is it intended to do anyway? > Initially, I used this to parse build history into the database. But now I am using a separate script- just forgot to remove this. Sorry. As for errors we will be throwing custom 404 for doesnotexist exceptions 3. installation statistics: > Thank you, I will look into this. > As Mojca said, I am not seeing any way to provide code review on Github > when it's already merged. Since you have the base application ready, it's > time to use PRs. I would also advise starting to follow at least some of > the PEP8 style guide conventions, it's good to follow clean code practices > from the beginning. You can either use coala lint or pylint before pushing > the code, if familiar. > I can submit PRs to the temporary repository Mojca mentioned about once it is available. We can then have a very fresh start. I will make the initial commit after improving the code based on the suggestions. Thanks Arjun
