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

Support custom subject text #231

Merged
merged 8 commits into from
Aug 7, 2024
Merged

Support custom subject text #231

merged 8 commits into from
Aug 7, 2024

Conversation

czy-29
Copy link
Contributor

@czy-29 czy-29 commented Jun 1, 2024

Close #230

czy-29 added a commit to opensound-org/opensound that referenced this pull request Jun 1, 2024
… the compact parameter to make the badge shorter

在deps.rs合并我”支持自定义subject文本“的pr之前,先使用compact参数让徽章短一点

ref: deps-rs/deps.rs#231
@Enet4 Enet4 self-requested a review June 1, 2024 08:21
@Enet4 Enet4 added the enhancement New feature or request label Jun 1, 2024
Copy link
Contributor

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

The feature works nicely in the general case. I did notice that there are no constraints to the subject content in terms of length and characters. Considering that any client can request these on the spot, it might be for the best that we add some safeguards (e.g. 100 maximum length).

Screenshot_2024-06-01_09-32-23

Also, subject is ignored if style is given an unsupported value.

@czy-29
Copy link
Contributor Author

czy-29 commented Jun 1, 2024

The feature works nicely in the general case. I did notice that there are no constraints to the subject content in terms of length and characters. Considering that any client can request these on the spot, it might be for the best that we add some safeguards (e.g. 100 maximum length).

Screenshot_2024-06-01_09-32-23

What do you think we should do when the length exceeds the limit? Should we treat it as if there is no subject parameter?

@czy-29
Copy link
Contributor Author

czy-29 commented Jun 1, 2024

Also, subject is ignored if style is given an unsupported value.

As for this issue, it seems to be an existing bug. Consider the following two requests:

They each have one parameter that is invalid, but neither of them responds to the other valid parameter.
The root of this problem seems to be that we can't simply get the extra_config through: serde_urlencoded::from_str::<ExtraConfigPartial>(qs).
We need to think about how to fix this.

@czy-29
Copy link
Contributor Author

czy-29 commented Jun 3, 2024

@Enet4
All the bugs have been gracefully fixed! The relevant rationales are written in the code comments.
The fix was a bit trickier than expected, but eventually I managed to find a graceful solution.
Looking forward to your review!

@czy-29
Copy link
Contributor Author

czy-29 commented Aug 4, 2024

@Enet4 Hello, two months have passed, can we merge this PR?

@robjtede
Copy link
Member

robjtede commented Aug 4, 2024

the same concept as label in shields.io,

is there any reason not to match the naming ?

src/server/mod.rs Outdated Show resolved Hide resolved
src/server/mod.rs Outdated Show resolved Hide resolved
@czy-29
Copy link
Contributor Author

czy-29 commented Aug 4, 2024

@robjtede Thanks for the review! I will make changes and reply tomorrow!

@czy-29
Copy link
Contributor Author

czy-29 commented Aug 5, 2024

the same concept as label in shields.io,

is there any reason not to match the naming ?

This is because we have been using the term subject instead of label for this concept in our internal code before. If we switch to label, it means overturning all the variable names in the previous code. I'm not sure if this is a good idea. Of course, exposing previous naming to the public can also cause confusion for users, so this is a trade-off, and I am not sure which option is better.

@czy-29
Copy link
Contributor Author

czy-29 commented Aug 5, 2024

@robjtede All modifications have been completed, waiting for your final decision on whether to use subject or label, and looking forward to your review!

Copy link
Member

@robjtede robjtede left a comment

Choose a reason for hiding this comment

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

i don't mind too much about the label, would be easy enough to add an alias later

@robjtede
Copy link
Member

robjtede commented Aug 7, 2024

thanks for your work on this 👍🏻

@robjtede robjtede merged commit 62891bb into deps-rs:main Aug 7, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support custom labels (to replace the hard-coded dependencies label)
3 participants