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

ENH: add finish milestone option. #655

Merged
merged 6 commits into from
Dec 9, 2020

Conversation

dragonyanglong
Copy link
Contributor

This PR closes #624
@sbillinge please review and merge.
BTW, how do you want the string argument to be put into report_summary? Currently I make it force to be an input, or we can change it to optional.

@codecov
Copy link

codecov bot commented Nov 30, 2020

Codecov Report

Merging #655 (b87da48) into master (c544721) will decrease coverage by 0.02%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #655      +/-   ##
==========================================
- Coverage   68.12%   68.10%   -0.03%     
==========================================
  Files          67       67              
  Lines        6705     6718      +13     
==========================================
+ Hits         4568     4575       +7     
- Misses       2137     2143       +6     
Impacted Files Coverage Δ
regolith/helpers/u_milestonehelper.py 70.51% <60.00%> (-1.52%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c544721...b87da48. Read the comment docs.

@sbillinge
Copy link
Contributor

This PR closes #624
@sbillinge please review and merge.
BTW, how do you want the string argument to be put into report_summary? Currently I make it force to be an input, or we can change it to optional.

Let's make it not required.

@sbillinge
Copy link
Contributor

One more thing. The a_projecta/u_projecta seems to have changed now that it dumps to .yaml instead of .yml which is what we have been using in the group databases. this is breaking everything now unfortunately. Is that a change that you might have made as a part of this checklist work? I need it fixed asap....

@sbillinge
Copy link
Contributor

One more thing. The a_projecta/u_projecta seems to have changed now that it dumps to .yaml instead of .yml which is what we have been using in the group databases. this is breaking everything now unfortunately. Is that a change that you might have made as a part of this checklist work? I need it fixed asap....

its really weird. I compared, for example, a_expense and this is working fine (reads expenses.yml then adds the new item to that same file and saves it) whereas a_projectum reads projecta.yml but writes out projecta.yaml. The code for rc.coll and rc.database seems to be identical between the two, so I am a bit confused at this point.... Can you see if you can figure it out?

@dragonyanglong
Copy link
Contributor Author

Hi Simon, if we want the "--finish <report_summary>" to be optional, we need to have two separate options, one to tell the program that we are doing finish flag (True or False), the other one to pass the report_summary str to the program. I was thinking about only using one optional arugment to do this, but couldn't figure out a logic solution in my mind.

@dragonyanglong
Copy link
Contributor Author

The yaml bug sounds quite strange. I checked all my five PRs to regolith repo so far.
#651 , #648 , #642 , #625 , and this one, it seems like my edits to regolith are only limited to the helper function .py file, not touching the dumping yet I guess. When did you notice this bug? Or any clues to help me better locate where this bug is from..

How would I see this bug? i.e. how to reproduce to see it is dumping to yaml or yml?, so I can try to revert to old versions one by one to see when this bug happens?

@dragonyanglong
Copy link
Contributor Author

@sbillinge ready for review and merge. As we discussed, --finish argument will simply just finish the milestones, won't need to people to input report_summary when finishing milestones.

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good.

  • I think it would be good to have a one-line print statement that says the name of the milestone, the prum and says has been finished.
  • need a news
  • it would be more useful yet if we could specify slices of indices. Not sure how to do that, but it would make it much better. e.g., 1-5 would close 1,2,3,4,5 and 1-5,7 would close 1,2,3,4,5,7. Don't spend a lot of time on writing a parser for this, but can we give an int or a list and use the slicing in python lists? so user has to type [0:4,6] for the above behavior?

@dragonyanglong
Copy link
Contributor Author

Thanks Simon. I updated. for "adding a news", do you mean write some news words when "--finish"? I think we have to create another argument like --news to do so. And, I thought we want a quick way to finish milestones, but if we request people to add a news, we cannot finish a list of milestones anymore, am I understanding correctly?

@sbillinge
Copy link
Contributor

sbillinge commented Dec 8, 2020 via email

@dragonyanglong
Copy link
Contributor Author

Got it. Done.

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple of comments.

@@ -0,0 +1,13 @@
**Added:** None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove None

@@ -0,0 +1,13 @@
**Added:** None
* A finish argument to finish milestone and record the end datetime.
* A list of indexes can be specified for the u_milestone helper.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plural of index is indices

help="Index of the item in the enumerated list to update. "
"If multiple indexes, separate by space",
type = int)
"Please enter in the format of `2,5,7` or `3-7`",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't quite clear. do you need left-leaning quotes or not? No spaces in the list? I guess you are going to parse as as string? Can I leave a single integer or do I have to do 2 for just closing one milestone? Preferrable would be if it is an int it will do just one in that case

@sbillinge sbillinge merged commit 1d15793 into regro:master Dec 9, 2020
@sbillinge
Copy link
Contributor

thanks @dragonyanglong this looks great. I will test!

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