Conversation
There was a problem hiding this comment.
- Please add figure names and refer them accordingly.
- It would be nice to see an example figure with verdicts other than pass and so users understand the problem where to look to fix.
- Why don't we italicized or boldfaced when referring to
fieldcomponent in filename fields?
I accidentally submitted my reviews. I am still reviewing so please wait, @taberger |
There was a problem hiding this comment.
@taberger It is nice to see actual examples and figures.
I left inline comments so you can take a look.
The main suggestions are
- add figure names and refer them accordingly: it would be nicer if you add annotations.
- add an example figure with verdicts other than pass and so users understand the problem where to look to fix.
- refer this tutorial to the main filename checker readme or move the content in that file.
- This tutorial isn't referenced anywhere so the ReadtheDocs cannot render this document so I couldn't check the URLs. It needs to be referenced in the main filename check readme somewhere. The best place seems to be https://mast-contributor-tools.readthedocs.io/en/latest/filename_check_readme.html#reading-the-results. Or you could modify the content as subsection in that section so it can be visible on the table of content on the right.
- Once you reference it or move the content in
filename_check_readme.md, then I can build the readthedocs and check the URLs. But for the future, I also want to show you how to build it on your own so next time you can try it yourself for your testing. To do so, you will need to create an account on Readthedocs. Anyway, we can discuss over slack or on a meeting.
Co-authored-by: astrojimig <87392865+astrojimig@users.noreply.github.com>
Co-authored-by: astrojimig <87392865+astrojimig@users.noreply.github.com>
Co-authored-by: Jinmi Yoon <jinmiyoon@users.noreply.github.com>
…l in official filename_checker readme, fixed some typos.
|
One thing that I just realized - I may need to update the tutorial result images and text based on the latest updates to dev since I started this PR. I'll check things out ASAP (before ~Friday) and then I'll ask for your final review, @jinmiyoon and @astrojimig |
|
@astrojimig and @jinmiyoon I've made the updates to the text as needed. One question I had - I noticed when looking back through some of the documentation in the various README files (filename_check_readme.md and sqlite_readme.md) that we may need to change some of our verbiage to match the way that the code actually works now. In particular, regarding NEEDS REVIEW/FAIL results. If that all looks good to you, then I think we're good to go, but I just wanted folks to put another eye on that while we have this MR open, because it may be worth making changes here while we're at it. In particular, notice how the hlsp_mct-tutorial_readme.txt gives a value_score of "fail" and severity "unrecognized" for the product_type field in the field evaluation table. This is different from what I had in my first push, where it was a value_score of "NEEDS REVIEW" with a severity "unrecognized". |
c87c064 to
a5178a4
Compare
|
A couple other things I noticed:
I'm wondering if either there are bugs, or you may be running an outdated version of the code. (Apologies that I don't have the time to test myself today.) |
|
@adrianlucy Thanks for looking into this. Yeah, something must have gone wrong. What's weird is I merged dev into this branch with commit hash |
|
@taberger what is the latest status of this PR? Based on your conversation with @adrianlucy and my trial to test-build the documentation, it seems that your sqlite tutorial doesn't seem to exist. Or I cannot find the sqlite readme on the newly build docs. Anyhow, I think we should tag up for building the docs so you can see what you need to change any docs if needed. Send me a calendar invite when you get a chance. |
…ev. Also modified checks for number of fields to < 4 instead of < 5.
…ent in potential_problems creation statement.
@jinmiyoon I've tested out the documentation commands for the sqlite tutorial (currently in docs/ top level directory). It isn't showing up in any trees, but if you search for it, you can find it. If you have a preferred place for it to be, let me know and I can move it to see if it shows up in the right spot. |
|
@taberger The |
|
@astrojimig I've implemented the change and now you can find the tutorial in the tree. Thank you!! Once @jinmiyoon is happy with the updates, I think we are good to merge! |
No description provided.