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: harvest URLs from Hypothes.is comments & higleted text #222

Merged
merged 3 commits into from
Mar 31, 2021

Conversation

ankostis
Copy link
Contributor

... plus a minor simplification-refactoring.

@karlicoss karlicoss merged commit ca61d96 into karlicoss:master Mar 31, 2021
@karlicoss
Copy link
Owner

Good idea, thanks!

@ankostis
Copy link
Contributor Author

ankostis commented Apr 1, 2021

I usually try to keep these helpers below 'index'

I follow call-dependency order, so that contradicted with the UX-order you follow - will keep that in mind next time, and put exclusively index() at the top, ok?

if there are multiple arguments of same type (like text/part_name), I usually force them to be keyword

I would rewrite this PR to inline the helper-function, and eliminate both nits, but now its been merged ¯\_(ツ)_/¯

@karlicoss
Copy link
Owner

karlicoss commented Apr 1, 2021

ill keep that in mind next time

Thanks! No problem anyway, it makes sense too! Like I said more of a nit.

I would rewrite this PR to inline the helper-function, and eliminate both nits, but now its been merged

IMO it's better to merge sooner than later for faster iteration if it doesn't break things :) But I can ask first next time if you prefer?

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