-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add blog post for improvements to data access #72
Conversation
cd07c5b
to
312c6d4
Compare
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.
request changes
b73da39
to
b7613a3
Compare
Yeah, just like in presentations, terminal with white background might actually be preferable here (despite the eyeball cancer :D)
Hm, weird, locally it aligns nicely. I'll investigate! |
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.
Are the calculation-dump*gif
used somewhere? Can you remove them if not used before merging
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.
LGTM, at least w.r.t new QueryBuilder
syntax section. Thanks :)
e82eb93
to
6fd7937
Compare
OK, I just applied the remaining changes and squashed the ~40 commits to clean up a bit, so this is ready for a final review, @agoscinski, @edan-bainglass, @giovannipizzi. |
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.
Some comments, and suggestions to slightly simplify some sentences
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.
Once Giovanni's comments are addressed, this is good to go from my end.
Co-authored-by: Giovanni Pizzi <[email protected]>
…text improvements
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.
OK, thanks for the review, @giovannipizzi. I have implemented all of your suggested changes, and further improved the text.
@khsrali linkcheck failing on some scientific URLs? |
I checked the failing url manually: And it works. |
Maybe this works Would put it in ignore if this does not work, one can use also the doi instead, that should never fail anyway |
EDIT: Now, all links (apart from https://www.go-fair.org) are working again. Seemed to really have been some general outage... @khsrali I'm just a bit confused because I guess it was working before. @agoscinski, good idea, I replaced it with the DOIs. I also removed the |
Meaning the process dumping feature and the more intuitive QB syntax.
EDIT: This should be ready for a first round of review. Thanks to @agoscinski's wisdom, I just changed the base of this PR to the branch of PR #69 on which it depends, so only the actual changes are shown. Requested review from @edan-bainglass as the author of the new QB syntax.
Direct link to the rendered version on RTD.