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

Make the ircbot url shortener configurable. #430

Merged
merged 4 commits into from Jul 12, 2017

Conversation

Projects
None yet
3 participants
@ralphbean
Contributor

ralphbean commented Jun 21, 2017

Someone can provide something like this in their configuration to make link
shortening work in... some other way::

config = dict(
    irc=[dict(
            make_short=lambda url: "http://awesome",
    )],
)
@ralphbean

This comment has been minimized.

Contributor

ralphbean commented Jun 21, 2017

@codecov-io

This comment has been minimized.

codecov-io commented Jun 21, 2017

Codecov Report

Merging #430 into develop will increase coverage by 1.07%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #430      +/-   ##
===========================================
+ Coverage    54.71%   55.79%   +1.07%     
===========================================
  Files           31       31              
  Lines         1908     1916       +8     
  Branches       300      301       +1     
===========================================
+ Hits          1044     1069      +25     
+ Misses         773      751      -22     
- Partials        91       96       +5
Impacted Files Coverage Δ
fedmsg/consumers/ircbot.py 50.69% <100%> (+10.02%) ⬆️

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 f530afb...d5347d3. Read the comment docs.

dagd = 'http://da.gd/s'
resp = requests.get(dagd, params=dict(url=link))
link = resp.text.strip()
if callable(short):

This comment has been minimized.

@jeremycline

jeremycline Jun 27, 2017

Member

Is the intention here to use some other shortener service? I ask because this approach is probably going to make error handling tricky since any sort of exception could be raised if someone opts to not use requests.

This comment has been minimized.

@ralphbean

ralphbean Jul 5, 2017

Contributor

Yes, that's the intention.

As a user - I'd like to still use requests, but just a different shortener service.

This comment has been minimized.

@jeremycline

jeremycline Jul 5, 2017

Member

Perhaps it would be better to just let a user configure the service URL and require that the service return the shortened link as the response body to a GET with the url parameter. Will that meet your needs?

@jeremycline

This comment has been minimized.

Member

jeremycline commented Jun 27, 2017

I haven't really investigated this particular consumer before, but I'm surprised it works. It seems to be using Twisted features outside the reactor thread.

@@ -66,6 +66,12 @@
}
def _default_link_shortener(url):
dagd = 'http://da.gd/s'
resp = requests.get(dagd, params=dict(url=url))

This comment has been minimized.

@jeremycline

jeremycline Jun 27, 2017

Member

This should definitely have a timeout kwarg applied to it.

This comment has been minimized.

@ralphbean

ralphbean Jul 5, 2017

Contributor

Can do.

ralphbean added some commits Jun 21, 2017

Make the ircbot url shortener configurable.
Someone can provide something like this in their configuration to make link
shortening work in... some other way::

```python
config = dict(
    irc=[dict(
            make_short=lambda url: "http://awesome",
    )],
)
```

@ralphbean ralphbean force-pushed the feature/configurable-shortener branch from 78efd58 to 11e3d48 Jul 5, 2017

@jeremycline

Could you please add tests (100% diff test coverage is required) and documentation on the new configuration option?

dagd = 'http://da.gd/s'
resp = requests.get(dagd, params=dict(url=link))
link = resp.text.strip()
if callable(short):

This comment has been minimized.

@jeremycline

jeremycline Jul 5, 2017

Member

Perhaps it would be better to just let a user configure the service URL and require that the service return the shortened link as the response body to a GET with the url parameter. Will that meet your needs?

@ralphbean

This comment has been minimized.

Contributor

ralphbean commented Jul 6, 2017

Perhaps it would be better to just let a user configure the service URL and require that the service return the shortened link as the response body to a GET with the url parameter. Will that meet your needs?

Unfortunately, no. :( I wish!

The shortener that I want (need) to use does accept GET requests, but it doesn't take a url parameter.

It is of the form:

https://shortener.com/new?the_url_goes_here

If it was like

https://shortener.com/new?url=the_url_goes_here

Then your approach would work.

ralphbean added some commits Jul 6, 2017

@ralphbean

This comment has been minimized.

Contributor

ralphbean commented Jul 6, 2017

Could you please add tests (100% diff test coverage is required) and documentation on the new configuration option?

Tests and docs attached.

@jeremycline

Thanks!

@jeremycline jeremycline merged commit 4171981 into develop Jul 12, 2017

4 checks passed

codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 55.79% (+1.07%) compared to f530afb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jeremycline jeremycline deleted the feature/configurable-shortener branch Jul 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment