twine extension #7

rzerres · 2021-03-22T22:23:14Z

twine-demo: add demo app

If you are working in a project, where code is organized in subtrees, i played with the best spots to include the macro. This little test app rebuilds a small structure that is supporting a library and binary part. Haven't got a clue how to preset the Language OS-agnostic, since Windows isn't using the default LANG approach. If you have a clue, this probably will improve the acceptence of the audiance.

use workspace
adopt test-crate

Greetings
Ralf

cecton

Hello!! 👋 Thanks a lot for joining the party!

So first let me answer your proposed improvements:

impl Default for Lang

Ok that sounds like a good idea but what should be the default language?

For what I read I think your intention is to use the language from the execution environment. Unfortunately I don't think this is possible as there is not a 1:1 mapping between the OS's specific language to the languages defined by Twine.

My current recommendation is that the user defines their default on their own. For example, this is perfectly legal to do:

// get the macro (t!) accessing the internationalization strings & the Lang enum!
include!(concat!(env!("OUT_DIR"), "/i18n.rs"));

// Please note that you *CAN* implement Default on Lang because Lang is NOT a foreign type.
impl Default for Lang {
    fn default() -> Self {
        Lang::En("")
    }
}

But maybe the users don't realize this is possible so I'm not against one of these solutions:

adding an example in examples showing it
adding an optional parameter "default_lang" to the functions that generate the code to allow optionally defining a default language
find a crate on crates.io that handles the "default" language well and provide some API to allow the user to do the mapping (this would need to go behind a feature flag because of the complexity and weight)
assuming the first language to appear is the default one, like this:

[err_lang_not_found]
	de = Konnte Sprachkode nicht auslesen
	en = Can not read the language code

☝️ de appears first so de is the default one.

newline corrections

Great, thanks!

correct linter hint (lifetime definition)

Great! ...I didn't get these linter hint issue... How did you get them? clippy? rustc? Do you have a screenshot?

Second: PR review itself.

I think your intention was to make an example crate and not a test crate. The difference is that the example crate is targeted to teach the user how to use and integrate the library in their project. The test crate focuses only on making sure the library works as intended.

src/lib.rs

twine-test/Cargo.toml

twine-test/src/main.rs

rzerres · 2021-03-29T00:51:19Z

My current recommendation is that the user defines their default on their own. For example, this is perfectly legal to do:

Fine, i wasn't aware of this. Good to know. And yes, this should be mentioned in the README.md

rzerres · 2021-03-29T00:55:49Z

* assuming the first language to appear is the default one, like this:

That would be my opt in.
If this isn't appropriate, user can impl Default for Lang {} as suggested.

cecton

* assuming the first language to appear is the default one, like this:
That would be my opt in.
If this isn't appropriate, user can impl Default for Lang {} as suggested.

The problem with that is that if twine does impl Default already for you, the user won't be able to do it.

I think it's best you remove the Default implementation and add a note in the crate's documentation instead so the user has maximum flexibility.

So here is a list of changes that sounds reasonable to me:

Update the crate's documentation. The README is generated automatically using cargo-readme
The example you provided need to be moved again to the directory examples/. The complete path is examples/twine-demo/
Add a Default impl for Lang in the example to illustrate how to do that
Simplify the example

cecton · 2021-03-29T07:23:14Z

twine-demo/src/main.rs

+        //println!{"lang: {}", &lang};
+
+        // include localization strings
+        let lang = Lang::De("de");


I find it weird that you are adding a Default impl but you don't seem to use it anywhere.

May I ask, do you have an actual use case where you need Lang::default()? Can you show me or explain

cecton · 2021-03-29T07:45:10Z

twine-demo/src/main.rs

+        //let mut lang = env::var("LANG").unwrap_or_else(|_| "en".to_string());
+        //let lang_culture = lang.substring(3,4).to_string();   // "de_DE.UTF-8" -> "DE"
+        //let lang_family = lang.substring(0,2).to_string();   // "de_DE.UTF-8" -> "de"
+        //lang = format!{"Lang::{}(\"{}\")", &lang_culture, &lang_family};
+        //println!{"lang: {}", &lang};


Remove all commented code

cecton · 2021-03-29T07:53:01Z

twine-demo/src/services/imports/twine_import.rs

+pub fn import(p: &mut String, lang: &Lang) -> Result<Duration, Box<dyn Error>> {
+    use std::path::Path;
+
+    let mut res = t!(import_started => lang);
+    let mut state = t!(state_started => lang);
+
+    let time_start = SystemTime::now();
+    trace!(target: "import", process = ?res, state = ?state,
+           date_start = ?time_start);
+
+    let path = Path::new(p);
+    assert_eq!(path.is_file(), true);
+    trace!(target: "import", path = ?path);
+
+    let mut child = Command::new("sleep").arg("5").spawn().unwrap();
+    let _result = child.wait().unwrap();
+
+    let mut time_end = SystemTime::now();
+    let duration = time_end
+        .duration_since(time_start)
+        .expect("Clock may have gone backwards");
+    trace!(target: "import", duration = ?duration);
+
+    state = t!(state_finished => lang);
+    res = t!(import_finished => lang);
+
+    time_end = SystemTime::now();
+    trace!(target: "import", process = ?res, state = ?state,
+           date_stop = ?time_end);
+
+    Ok(duration)
+}


Please do not use p. The languages are supposed to be loaded at compile time and it is very confusing that you are now checking something with the .rs file that has been generated.

cecton · 2021-03-29T07:53:32Z

twine-demo/src/main.rs

+        trace!(target: "twine-demo", state = ?state,
+               res = ? res, time = ?time_start);
+
+        let mut import_path = concat!(env!("OUT_DIR"), "/i18n.rs").to_string();


Don't do that

I mean this file is not meant to be read at runtime. It makes no sense to do this.

You are right. Will clean it up and rebase the twine-demo code to be more intutive.

* impl Default for Lang * newline corrections * correct linter hint (lifetime definition) Signed-off-by: Ralf Zerres <[email protected]>

* use workspace * adopt test-crate Signed-off-by: Ralf Zerres <[email protected]>

cecton · 2021-06-11T17:10:08Z

src/lib.rs

+        sorted_languages.sort_unstable();
+
+        let (default_lang, default_region) = sorted_languages[0];


You're still taking the first language in alphabetical order. This is not good.

Yeh,
didn't go into it. Actually i might just get rid of a default branch. User must choose explicitely for the language to choose.
I might implement something in the twine-demo code ....

Sounds good to me

* choose first member of sorted languages as proposte default lang * rename `twine-test` to `twine-demo` * since `twine-demo` is a crate, it will be handled as a workspace examples can't be build using a Cargo.toml * update Cargo.toml to meet the corrections * update README.md Signed-off-by: Ralf Zerres <[email protected]>

Signed-off-by: Ralf Zerres <[email protected]>

* if compile call activates macro "missing_docs" provide suitable documentation strings Signed-off-by: Ralf Zerres <[email protected]>

cecton requested changes Mar 24, 2021

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

twine-test/Cargo.toml Outdated Show resolved Hide resolved

twine-test/src/main.rs Outdated Show resolved Hide resolved

rzerres force-pushed the wip_twine branch from 93c1474 to 0dc46cb Compare March 29, 2021 01:56

cecton requested changes Mar 29, 2021

View reviewed changes

rzerres added 2 commits June 11, 2021 17:37

lib.rs: i18n.rs enhancement

7331b19

* impl Default for Lang * newline corrections * correct linter hint (lifetime definition) Signed-off-by: Ralf Zerres <[email protected]>

twine-test: add test app

9466d18

* use workspace * adopt test-crate Signed-off-by: Ralf Zerres <[email protected]>

rzerres force-pushed the wip_twine branch from 0dc46cb to ff7fe20 Compare June 11, 2021 15:45

cecton reviewed Jun 11, 2021

View reviewed changes

rzerres force-pushed the wip_twine branch 3 times, most recently from 4c63c8f to e3901bd Compare June 12, 2021 23:35

rzerres added 2 commits June 13, 2021 01:42

twine-demo: update example code

bb74f08

Signed-off-by: Ralf Zerres <[email protected]>

rzerres force-pushed the wip_twine branch from e3901bd to bb74f08 Compare June 12, 2021 23:43

lint: documentation strings

ca09ac6

* if compile call activates macro "missing_docs" provide suitable documentation strings Signed-off-by: Ralf Zerres <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

twine extension #7

twine extension #7

rzerres commented Mar 22, 2021 •

edited

Loading

cecton left a comment

rzerres commented Mar 29, 2021

rzerres commented Mar 29, 2021

cecton left a comment •

edited

Loading

cecton Mar 29, 2021

cecton Mar 29, 2021

cecton Mar 29, 2021

cecton Mar 29, 2021

cecton Apr 2, 2021

rzerres Jun 13, 2021

cecton Jun 11, 2021

rzerres Jun 13, 2021

cecton Jun 14, 2021

		sorted_languages.sort_unstable();

		let (default_lang, default_region) = sorted_languages[0];

twine extension #7

Are you sure you want to change the base?

twine extension #7

Conversation

rzerres commented Mar 22, 2021 • edited Loading

Contents

lib.rs: enhancements

twine-demo: add demo app

cecton left a comment

Choose a reason for hiding this comment

rzerres commented Mar 29, 2021

rzerres commented Mar 29, 2021

cecton left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzerres commented Mar 22, 2021 •

edited

Loading

cecton left a comment •

edited

Loading