-
Notifications
You must be signed in to change notification settings - Fork 70
style: add builder for releaselist. #1273
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1273 +/- ##
==========================================
+ Coverage 79.64% 79.86% +0.22%
==========================================
Files 17 17
Lines 1366 1381 +15
==========================================
+ Hits 1088 1103 +15
Misses 278 278
🚀 New features to boost your workflow:
|
sbillinge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will review the code shortly but we need to add this to builder.py so the tests fail
sbillinge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. See typo
sbillinge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just found this unsubmitted review....sorry about that. I mentioned some of the things at group meeting yesterday, but there are probably a few more. Please see below.
This is already good. I think it may not work out of the box because in the presentations, each presentation has a date, but here we are iterating over programs in softare, and we then have to further iterate over releases in program to get dates, so that may take more work, unless you already did that.
I don't see the output template here. The way to build that is to take the tex file from the test and replace the text with jinja template code and iterators that will build the lists.
Good progress!
| gtx["grants"] = sorted(all_docs_from_collection(rc.client, "grants"), key=_id_key) | ||
| gtx["groups"] = sorted(all_docs_from_collection(rc.client, "groups"), key=_id_key) | ||
| gtx["software"] = sorted(all_docs_from_collection(rc.client, "software"), key=_id_key) | ||
| gtx["institutions"] = sorted(all_docs_from_collection(rc.client, "institutions"), key=_id_key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if we will use this one. I don't remember any institutions in the output file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: Checked, we don't need this, I would at least delete the code in tools.py that uses institutions. But we actually have one called institutions.yaml in the output template folder.
| everybody, self.gtx["software"], self.gtx["institutions"], member, statuses=["active"] | ||
| ) | ||
|
|
||
| if len(presclean) > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may want to change presclean everywhere to progclean or sthg like that.
src/regolith/tools.py
Outdated
|
|
||
| # build the filtered collection | ||
| # only list the talk if the group member is an author | ||
| for sof in software: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for program in software: for greater readability
src/regolith/tools.py
Outdated
| authorids = [author["_id"] if author is not None else author for author in authors] | ||
| if target in authorids: | ||
| firstclean.append(sof) | ||
| # only list the presentation if it has status in statuses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change presentation here to program
src/regolith/tools.py
Outdated
| sof["{}day_suffix".format(day)] = number_suffix(get_dates(sof).get(f"{day}date").day) | ||
| except AttributeError: | ||
| print(f"presentation {sof.get('_id')} has no {day}date") | ||
| if "institution" in sof: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need any institution informatoin so this can be deleted
news/releasebuilder.rst
Outdated
| **Added:** | ||
|
|
||
| * Add test output for releaselist. | ||
| * Add Releaselistbuilder functionality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check capitalization. Probably "Add Releaselistbuilder functionality"
Please check but I don't think we punctuate changelog entries
sbillinge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused why this is passing tests. Should it not be failing?
Originally it fails, but after I commited for the filter function in |
|
I changed the |
|
Ok, maybe add a minimal template and see if we can get the test to fail |
|
@sbillinge Got it why it would always pass it. Is that because in the original CI test we tested the original ones that has no coverage on |
it should test it by trying to build the doc. I am guessing that it doesn't pass because there is no output template and so it actually doesn't render anything. So just put a template file a template folder and I think it should start failing |
|
@sbillinge Actually I should have a output template called |
|
@sbillinge I noticed that we have a Update: Have added the test on ReleaseListBuilder in More update: Now the test gives the error: which means we are not passing it to the command of build, I'm not sure where we need to add that? need some clarification on this. |
|
great progress....we have a failing test! :) The test is not finding the rendered files. Either they are not being rendered or they are appearing in a different place or with a different tname than you used in your test output. By default they go to |
|
I see progress here. Please let me know what you need from me? |
|
@sbillinge I think right now it goes fairly well here. The status right now is it now renders the latex file, but there is a mismatch between the built and the template. So, the next step is to fix those mismatch. |
|
@stevenhua0320 great progress. The code goes into different collections in the database to get different information, so we have to put things into our software database that exist in other databases (like aeinstein....I see you found that). But yes, please just push on and get it passing if you can and I can merge. Please let me know if some things are causing issues. Working with the jinja is a bit of a pain and I may be able to give advice if you paste images of things going wrong into the comments. |
|
@sbillinge Finally! It passed all the tests. Ready to review |
|
yeh! |
@sbillinge Ready for initial review, I should further add
filter_softwaresimilar style asfilter_presentationintools.pyand addbtypeintobuilder.py. But for now, we should first confirm whether I am on the right track.