Replace the input! macro with an Input builder #646

已合併
elegaanz 將 4 次提交從 input-builder 合併至 master 5 年前
elegaanz 已留言 5 年前 (已從 github.com 遷移)

Pros:

  • less ambiguities
  • more type safety
  • less cloning (we don't clone forms for each input anymore)

Cons:

  • more verbose (but at the same time it is easier to parse IMO)
  • less natural syntax
Pros: - less ambiguities - more type safety - less cloning (we don't clone forms for each input anymore) Cons: - more verbose (but at the same time it is easier to parse IMO) - less natural syntax
codecov[bot] 已留言 5 年前 (已從 github.com 遷移)

Codecov Report

Merging #646 into master will decrease coverage by 0.26%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #646      +/-   ##
==========================================
- Coverage   35.06%   34.79%   -0.27%     
==========================================
  Files          68       68              
  Lines        7945     8006      +61     
  Branches     1890     1894       +4     
==========================================
  Hits         2786     2786              
- Misses       4383     4444      +61     
  Partials      776      776
# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/646?src=pr&el=h1) Report > Merging [#646](https://codecov.io/gh/Plume-org/Plume/pull/646?src=pr&el=desc) into [master](https://codecov.io/gh/Plume-org/Plume/commit/a6c84daa1a242246ac01260eb9bc201dd6e47830?src=pr&el=desc) will **decrease** coverage by `0.26%`. > The diff coverage is `0%`. ```diff @@ Coverage Diff @@ ## master #646 +/- ## ========================================== - Coverage 35.06% 34.79% -0.27% ========================================== Files 68 68 Lines 7945 8006 +61 Branches 1890 1894 +4 ========================================== Hits 2786 2786 - Misses 4383 4444 +61 Partials 776 776 ```
igalic (已從 github.com 遷移) 已審核 5 年前
igalic (已從 github.com 遷移) 留下了回應

this looks pretty cool!

this looks pretty cool!
擁有者

Maybe implementing https://docs.rs/ructe/0.6.2/ructe/templates/trait.ToHtml.html would make more sens than a stand-alone function? Also, clippy is complaining about conventions, to_* usually take by reference

Maybe implementing https://docs.rs/ructe/0.6.2/ructe/templates/trait.ToHtml.html would make more sens than a stand-alone function? Also, clippy is complaining about conventions, to_* usually take by reference
elegaanz 已留言 5 年前 (已從 github.com 遷移)

I thought about it, but my function takes a gettex::Catalog, while ToHtml::to_html doesn't. I could have passed it as an argument to Input::new tho.

But I made the commit before reading your review, so it is Input::html now (I can still change it if you prefer).

I thought about it, but my function takes a `gettex::Catalog`, while `ToHtml::to_html` doesn't. I could have passed it as an argument to `Input::new` tho. But I made the commit before reading your review, so it is `Input::html` now (I can still change it if you prefer).
igalic 已留言 5 年前 (已從 github.com 遷移)

definitely, what @fdb-hiroshima said


update / carification:
About implementing Ructe's ToHtml trait.

definitely, what @fdb-hiroshima said ****** update / carification: About implementing Ructe's `ToHtml` trait.
擁有者

I did not notice the Catalog. It was just a suggestion, but as it's totally fine as is

I did not notice the Catalog. It was just a suggestion, but as it's totally fine as is
擁有者

As .html() take the Catalog, and the provided label in Input::new is always translated, can't it be translated transparently from .html() instead of explicitly each time? (I don't know if it would be better or worst, that's an actual question)

As `.html()` take the Catalog, and the provided label in Input::new is _always_ translated, can't it be translated transparently from `.html()` instead of explicitly each time? (I don't know if it would be better or worst, that's an actual question)
elegaanz 已留言 5 年前 (已從 github.com 遷移)

gettext-macros requires strings that are known at compile time, so that it can add them to the .pot file. With the latest version of the crate (which requires Rust 2018 for some reason, so we can't use it now), there is a new macro, called t that would allow us to do:

Input::new("email", t!("Your email address"))
    .input_type("email")
    .html(catalog);

Which would be a bit better (but we can't really do anything else than that I think, these strings should be marked as translatable with a macro at some point).

gettext-macros requires strings that are known at compile time, so that it can add them to the .pot file. With the latest version of the crate (which requires Rust 2018 for some reason, so we can't use it now), there is a new macro, called `t` that would allow us to do: ```rust Input::new("email", t!("Your email address")) .input_type("email") .html(catalog); ``` Which would be a bit better (but we can't really do anything else than that I think, these strings should be marked as translatable with a macro at some point).
trinity-1686a 已審核 5 年前
trinity-1686a 留下了回應
擁有者

👀

👀
擁有者

I think error is no longer put in a <p class="error" dir="auto">{}</p>

I think error is no longer put in a `<p class="error" dir="auto">{}</p>`
@ -13,0 +13,4 @@
.default(&form.title)
.error(&errors)
.set_prop("minlength", 1)
.html(ctx.1))
擁有者

previously had the requiredproperty

previously had the `required`property
@ -18,3 +18,3 @@
<form method="post" action="@uri!(instance::update_settings)">
@input!(ctx.1, name (text), "Name", form, errors.clone(), "props")
@(Input::new("name", i18n!(ctx.1, "Name"))
擁有者

what was props, why is it gone?

what was `props`, why is it gone?
trinity-1686a 已審核 5 年前
@ -13,0 +13,4 @@
.default(&form.title)
.error(&errors)
.set_prop("minlength", 1)
.html(ctx.1))
擁有者

mb, it's added automagically when not optional

mb, it's added automagically when not optional
elegaanz (已從 github.com 遷移) 已審核 5 年前
@ -18,3 +18,3 @@
<form method="post" action="@uri!(instance::update_settings)">
@input!(ctx.1, name (text), "Name", form, errors.clone(), "props")
@(Input::new("name", i18n!(ctx.1, "Name"))
elegaanz (已從 github.com 遷移) 已留言 5 年前

I don't know to be honest, but it clearly was here by mistake (this argument was for additional HTML properties).

I don't know to be honest, but it clearly was here by mistake (this argument was for additional HTML properties).
trinity-1686a 核可了這些變更 5 年前
trinity-1686a 留下了回應
擁有者

👍

👍

審核者

trinity-1686a 核可了這些變更 5 年前
此合併請求已被合併為 8ab690001d
您也可以查看命令列指南

第一步:

在您的儲存庫中切換到新分支並測試變更。
git checkout -b input-builder master
git pull origin input-builder

第二步:

合併變更並更新到 Forgejo。
git checkout master
git merge --no-ff input-builder
git push origin master
登入 才能加入這對話。
沒有審核者
未選擇里程碑
沒有負責人
2 參與者
通知
截止日期
截止日期無效或超出範圍,請使用「yyyy-mm-dd」的格式。

未設定截止日期。

先決條件

未設定先決條件。

參考: Plume/Plume#646
載入中…
尚未有任何內容