Change config file format from YAML to TOML

[Svenskunganka]

Some things to note:

• Unknown fields in the config file results in an error. Can easily be changed by removing #[serde(deny_unknown_fields)] from the structs.
• Unset environment variables results in an error.
• Environment variables are only accepted in a ${VARIABLE}/${variable} format, not $VARIABLE as that's going to limit users via case-sensitivity: /path/to/$VARIABLETemplates won't work while /path/to/${VARIABLE}Templates will.
• Variable name case-sensitivity is platform-specific and retains whatever default functionality std::env has:
  ${VaRiAbLe} != ${VARIABLE} != ${variable}

Things I'm unsure about:

• How to add more tests than what was already available as I suspect I'd have to mark most of ConfigFile as pub to access from sailfish-tests crate (and add some more methods)

• How the error macros & conversions works. I think I've managed to get the errors descriptive. E.g:

  ---- read_config stdout ----
  thread 'read_config' panicked at 'called `Result::unwrap()` on an `Err` value: Error { source_file: Some("/home/svenskunganka/dev/rust/sailfish/sailfish-tests/integration-tests/config/sailfish.toml"), source: None, offset: None, chains: [ConfigError("Environment variable (MYVARIABLE) not set")] }', sailfish-tests/integration-tests/tests/config.rs:7:55
  
  ---- read_config stdout ----
  thread 'read_config' panicked at 'called `Result::unwrap()` on an `Err` value: Error { source_file: Some("/home/svenskunganka/dev/rust/sailfish/sailfish-tests/integration-tests/config/sailfish.toml"), source: None, offset: None, chains: [ConfigError("unknown field `key_not_exist`, expected one of `template_dirs`, `delimiter`, `escape`, `optimizations` at line 6 column 1")] }', sailfish-tests/integration-tests/tests/config.rs:7:55

Hopefully this time I've managed to not miss something like I did in my last PR.

Closes #44
Closes #42
Closes #38

[vthg2themax]

@Svenskunganka Have you recently tested this after all the latest updates? I like the idea conceptually of matching the TOML format that rust already uses for cargo, but will need to thoroughly test this to ensure there is no breakage. I am still getting the hang of this codebase, so I appreciate your patience! @Kogia-sima, @jdrouet what do you think?

[jdrouet]

@vthg2themax I agree with you about the toml format.

[vthg2themax]

@jdrouet @Svenskunganka I am looking at it, and it looks good, however I want to run the files locally to test it. Aside from manually copying the changes to my local copy, do you know of a way in git or something to get these changes? Thanks for any help you can offer!

[jdrouet]

Something like this should do it

git remote add sven https://github.com/Svenskunganka/sailfish.git
git fetch sven
git checkout sven toml

[vthg2themax]

@jdrouet Thanks! I thought I would have to use the GitHub desktop client on a windows box.

[vthg2themax]

@jdrouet After running cargo update, and cargo build, I'm getting errors while compiling it. I'll try to see what else I can come up with.

[vthg2themax]

@Svenskunganka @jdrouet I think perhaps the original person offering this change is not available to pull the latest changes into their branch? If so, perhaps manually copying the changes would be the best. It looks like they are using an older copy of sailfish.

[jdrouet]

@vthg2themax you can push the branch to your own fork and merge master into it 😉

[vthg2themax]

@jdrouet Well, here goes. I kind of think that the Merge Pull request button should not break anything in master, but if it does, I'll figure it out. Thank you for your help!

[Svenskunganka]

Hey guys, sorry for not getting back earlier. Cool that this is now merged!

Some thing to note that we might need to look into before publishing a new version:

Currently, after expanding environment variables - the path is not resolved, e.g /path/to/${MYVAR}_stuff/templates where MYVAR contains ../../ expands to /path/to/../../_stuff/templates and not /_stuff/templates which you might expect.
I don't think this is an issue because when sailfish compiler attempts to read a template from that path and it doesn't exist, it will eventually error once fallbacks has been exhausted and it couldn't find a template source file, which is a good thing because we let the OS take care of resolving the path for us. But I'd like to know your opinion on this.

You can see more of this in the tests I added.

Resolving the template:

sailfish/sailfish-compiler/src/procmacro.rs

Lines 85 to 104 in ef48042

	fn resolve_template_file(path: &str, template_dirs: &[PathBuf]) -> Option<PathBuf> {
	for template_dir in template_dirs.iter().rev() {
	let p = template_dir.join(path);
	if p.is_file() {
	return Some(p);
	}
	}
	let mut fallback = PathBuf::from(std::env::var("CARGO_MANIFEST_DIR").expect(
	"Internal error: environmental variable `CARGO_MANIFEST_DIR` is not set.",
	));
	fallback.push("templates");
	fallback.push(path);
	if fallback.is_file() {
	return Some(fallback);
	}
	None
	}

Errors out here:

sailfish/sailfish-compiler/src/procmacro.rs

Lines 196 to 208 in ef48042

	let input_file = {
	let path = all_options.path.as_ref().ok_or_else(|| {
	syn::Error::new(Span::call_site(), "`path` option must be specified.")
	})?;
	resolve_template_file(&*path.value(), &*config.template_dirs).ok_or_else(
	|| {
	syn::Error::new(
	path.span(),
	format!("Template file {:?} not found", path.value()),
	)
	},
	)?
	};

The error here could be improved to tell the user where the compiler has tried looking for the template but couldn't find it, which will help users find out if their template_dirs definition is wrong or the contents of their embedded environment variable is wrong.
The implementation will need to be migrated from Path::is_file to something else as it coerces errors to false though.

Finally, this is a breaking change and probably warrants a 0.4 version bump before release.