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.

Sign up for GitHub

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

Jump to bottom

Breaking changes for 5.0 release #93

Merged
merged 5 commits into from
Nov 18, 2022
Merged

Breaking changes for 5.0 release #93

merged 5 commits into from
Nov 18, 2022

Conversation

kevindew
Copy link
Member

@kevindew kevindew commented Oct 3, 2022

Since we have a 5.0 release looming: #92 I thought it'd be good to resolve the remaining TODO's in this repo and release other breaking changes.

This change does a few things:

  1. It removes Plek.current, this is likely the most impactful change, as despite this being deprecated 10 years ago it hasn't 100% filtered through the stack. I'm expecting this can be easily resolved with upgrading to this gem - although I'm going to push out a change for gds-api-adapters first.
  2. It removes the methods that return URI objects, these were inconsistently implemented since external_url_for was added and seem a bit unnecessary since they're barely used and just a wrapper for URI(Plek.find('x'))
  3. It makes the static method interface on Plek consistent.

kevindew added a commit that referenced this pull request Oct 3, 2022
@kevindew
Deprecate the use of Plek.current
ab0bcfb 
This method was put in temporarily 10 years ago [1] and has had a todo
next to it ever since. We're planning to finally remove this [2] in the
5.0 release so this acts as a warning in the interim.

[1]: 309ef59
[2]: #93
@kevindew kevindew mentioned this pull request Oct 3, 2022
Merged
kevindew added a commit that referenced this pull request Oct 3, 2022
@kevindew
Deprecate the use of Plek.current
56242a3 
This method was put in temporarily 10 years ago [1] and has had a todo
next to it ever since. We're planning to finally remove this [2] in the
5.0 release so this acts as a warning in the interim.

[1]: 309ef59
[2]: #93
sengi
sengi approved these changes Oct 3, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
lib/plek.rb Outdated Show resolved Hide resolved
lib/plek.rb Show resolved Hide resolved
@sengi
Copy link
Contributor

sengi commented Oct 3, 2022

Thanks for the cleanup!

@kevindew
Remove Plek.current method
8cf99b9 
This method was deprecated back in October 2012 [1]. This now removes
the method and resolves this todo.

It's unfortunate that there was not a warning for the use of this
method, but as it's been such a long time and that we're working towards
a new major version this feels appropriate.

[1]: 309ef59
@kevindew
Remove URI based look up methods
3fa8a68 
These seemed unnecessary as it's just running URI over the returned
argument and clients can do that. These had fallen out of sync with the
interface the class actually had so it seemed best to remove them.

From my GitHub search there's only a couple of places these are used in
our code.
@kevindew
Have a consistent static interface
d63a7fe 
This puts all the instance methods as static methods so that a plek
instance and Plek static are consistent in their behaviour.

This also updates the previous change note for the method suggestions.
@kevindew kevindew force-pushed the remove-old-interface branch 2 times, most recently from 257afdc to e364af6 Compare October 3, 2022 16:12
@kevindew
Reject malformed service names
b71de49 
Previously this tool was very forgiving in what user input it accepted
and this meant that some really horribly wrong stuff could be attempted
to be salvaged. For example:

```
irb(main):005:0> Plek.find(" S76I%Gn?O^n    ")
=> "http://signon.dev.gov.uk"
```

Which after this change now does:

```
irb(main):001:0> Plek.find(" S76I%Gn?O^n    ")
Traceback (most recent call last):
       16: from /Users/kevin.dew/.rbenv/versions/2.7.6/bin/bundle:23:in `load'
       15: from /Users/kevin.dew/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/bundler-2.1.4/libexec/bundle:34:in `<top (required)>'
       14: from /Users/kevin.dew/.rbenv/versions/2.7.6/lib/ruby/2.7.0/bundler/friendly_errors.rb:123:in `with_friendly_errors'
       13: from /Users/kevin.dew/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/bundler-2.1.4/libexec/bundle:46:in `block in <top (required)>'
       12: from /Users/kevin.dew/.rbenv/versions/2.7.6/lib/ruby/2.7.0/bundler/cli.rb:24:in `start'
       11: from /Users/kevin.dew/.rbenv/versions/2.7.6/lib/ruby/2.7.0/bundler/vendor/thor/lib/thor/base.rb:476:in `start'
       10: from /Users/kevin.dew/.rbenv/versions/2.7.6/lib/ruby/2.7.0/bundler/cli.rb:30:in `dispatch'
        9: from /Users/kevin.dew/.rbenv/versions/2.7.6/lib/ruby/2.7.0/bundler/vendor/thor/lib/thor.rb:399:in `dispatch'
        8: from /Users/kevin.dew/.rbenv/versions/2.7.6/lib/ruby/2.7.0/bundler/vendor/thor/lib/thor/invocation.rb:127:in `invoke_command'
        7: from /Users/kevin.dew/.rbenv/versions/2.7.6/lib/ruby/2.7.0/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
        6: from /Users/kevin.dew/.rbenv/versions/2.7.6/lib/ruby/2.7.0/bundler/cli.rb:506:in `console'
        5: from /Users/kevin.dew/.rbenv/versions/2.7.6/lib/ruby/2.7.0/bundler/cli/console.rb:19:in `run'
        4: from (irb):1
        3: from /Users/kevin.dew/.rbenv/versions/2.7.6/lib/ruby/2.7.0/forwardable.rb:235:in `find'
        2: from /Users/kevin.dew/govuk/plek/lib/plek.rb:75:in `find'
        1: from /Users/kevin.dew/govuk/plek/lib/plek.rb:149:in `valid_service_name'
ArgumentError (Plek expects a service name only contain lowercase a-z, . (period) and - (dash) characters.)
```

This changes the behaviour so that it accepts only hostnames in
lowercase with the expected characters and doesn't try to make fixes. I
also expanded it to include numbers as this seemed an unusual omission.

The expectation is that this will have a negligible effect on real code
as it's unlikely that our committed code is not already valid.
@kevindew
Prefer convenience methods in tests
efa7df5 
Since the docs utilise the convenience methods it makes sense that our
tests prefer them over the extra init step.
@kevindew kevindew force-pushed the remove-old-interface branch from e364af6 to efa7df5 Compare October 3, 2022 16:17
@kevindew
Copy link
Member Author

kevindew commented Oct 3, 2022

Thanks for the review @sengi, some great spots! I'll probably wait a week or two to merge this so the deprecations has gone through.

@MuriloDalRi
Copy link
Contributor

Hi @kevindew do you think we can merge this now?

@kevindew kevindew changed the title Other breaking changes for 5.0 release Breaking changes for 5.0 release Nov 18, 2022
@kevindew
Copy link
Member Author

@MuriloDalRi Yup, I can see a few apps will need a tweak for this (Whitehall, Short URL Manager, Link Checker API) but looks straightforward.

@kevindew kevindew merged commit e58f3bf into main Nov 18, 2022
@kevindew kevindew deleted the remove-old-interface branch November 18, 2022 12:16
@kevindew
Copy link
Member Author

@MuriloDalRi I've opened #96 to release it

This was referenced Nov 21, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sengi sengi sengi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kevindew @sengi @MuriloDalRi