Skip to content

Comments

feat(server): allow named url shortening (#40)#373

Merged
orhun merged 1 commit intoorhun:masterfrom
Vaelatern:enable-named-url-shortening
Dec 7, 2024
Merged

feat(server): allow named url shortening (#40)#373
orhun merged 1 commit intoorhun:masterfrom
Vaelatern:enable-named-url-shortening

Conversation

@Vaelatern
Copy link
Contributor

@Vaelatern Vaelatern commented Dec 5, 2024 •
edited
Loading

Description

I ran the filename header through the URL shortening route

Motivation and Context

This lets me use rustypaste to link to a specific link using a specified short name (for ergonomics).

Also fixes #40 by the by.

How Has This Been Tested?

Changelog Entry

### Added

- Enable naming shortened URLs beyond using a random string or the text "url"

-->

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (no code change)
  • Refactor (refactoring production code)
  • Other

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.
  • I have formatted the code with rustfmt.
  • I checked the lints with clippy.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@tessus
Copy link
Collaborator

tessus commented Dec 5, 2024

Thanks for the PR. Can you also please add test cases and fixtures?

@Vaelatern Vaelatern force-pushed the enable-named-url-shortening branch from 00153af to ca2ff07 Compare December 5, 2024 23:07
@Vaelatern
Copy link
Contributor Author

Vaelatern commented Dec 5, 2024

Sure thing! I'm trying to get in lint compliance too.

@tessus
Copy link
Collaborator

tessus commented Dec 5, 2024

There are a few things to fix. The formatting checks, lints, and tests fail. These have to be fixed and new tests should be added that handle your use case.

@Vaelatern Vaelatern force-pushed the enable-named-url-shortening branch 3 times, most recently from 6fdb015 to d718a84 Compare December 5, 2024 23:18
tessus
tessus requested changes Dec 5, 2024
View reviewed changes
Copy link
Collaborator

@tessus tessus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately there are still issues with this PR.

May I suggest to go through the checklist?

  • cargo fmt --check --
  • cargo clippy
  • cargo test -- --test-threads 1
  • ./fixtures/test-fixtures.sh

Currently the code does not even compile.

@Vaelatern Vaelatern force-pushed the enable-named-url-shortening branch from d718a84 to c9c0614 Compare December 6, 2024 00:57
@Vaelatern
Copy link
Contributor Author

Vaelatern commented Dec 6, 2024 •
edited
Loading

I've pushed an update now, it passes CI on my own fork. I've also updated the checkboxes in the original message with what I've done.

@Vaelatern Vaelatern mentioned this pull request Dec 6, 2024
Merged
12 tasks
tessus
tessus approved these changes Dec 6, 2024
View reviewed changes
Copy link
Collaborator

@tessus tessus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this looks good. Thanks.

@tessus
Copy link
Collaborator

tessus commented Dec 6, 2024

@orhun can you please have a look? I promised I would only merge dependency PRs. ;-)

@Vaelatern let's wait for orhun's approval, since he's the owner of the project. I can't merge without his ok.

@Vaelatern
Copy link
Contributor Author

Vaelatern commented Dec 6, 2024

Sounds good!

I just started using this today, noticed this hole, and wanted to contribute a fix.

@Vaelatern
Copy link
Contributor Author

Vaelatern commented Dec 6, 2024

Apparently I need to sign the commits too

@tessus
Copy link
Collaborator

tessus commented Dec 6, 2024

why? nope, you don't. This project does not have a requirement of signed commits.

@tessus
Copy link
Collaborator

tessus commented Dec 6, 2024

oops, yeah. I am sorry. this policy must be new.

@Vaelatern Vaelatern force-pushed the enable-named-url-shortening branch from c9c0614 to e390f46 Compare December 6, 2024 02:01
@Vaelatern
Copy link
Contributor Author

Vaelatern commented Dec 6, 2024

Now pushed with signed commit, both this and #374 (which I'm about to refactor a tiny bit)

@tessus
Copy link
Collaborator

tessus commented Dec 6, 2024 •
edited
Loading

Thanks.

P.S.: We squash the commits in a PR anyway, in which case the resulting commit would be signed, so the message under the merge button was a bit confusing and IMO wrong. It should say that all commits in the PR must be signed.

orhun
orhun approved these changes Dec 7, 2024
View reviewed changes
Copy link
Owner

@orhun orhun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff 💯

Thanks!

@orhun orhun merged commit 03dc194 into orhun:master Dec 7, 2024
@Vaelatern Vaelatern deleted the enable-named-url-shortening branch December 8, 2024 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tessus tessus tessus approved these changes

@orhun orhun orhun 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.

Only one URL at a time can be shortened when filenames aren't random

3 participants

@Vaelatern @tessus @orhun