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

[WIP] Lazy interface #3

Closed
wants to merge 12 commits into from
Closed

[WIP] Lazy interface #3

wants to merge 12 commits into from

Conversation

CAD97
Copy link
Collaborator

@CAD97 CAD97 commented Apr 5, 2018

As it turns out, the macro interface is probably possible using just macro-by-example, even if it has a few shortcomings (read: the #[ci] has to come before the doc comments).

I still need to convert the other added providers (or at least AppVeyor and Travis) to the new format, but everything should transfer over, though I'll probably have to flatten Jenkins' GHPRB.

Does this shape meet your use case, @epage ? We can add a trait on top and provide a ci_info() -> dyn CommonCIInfo for portable information.

@CAD97 CAD97 requested a review from epage April 5, 2018 15:59
@CAD97
Copy link
Collaborator Author

CAD97 commented Apr 5, 2018

I've also added examples/circle and plan to do the rest in order that we can run the example on CI to make sure that it doesn't panic when we're on that CI.

@CAD97
Copy link
Collaborator Author

CAD97 commented Apr 6, 2018

OK, this needs a rebase because of the several complete refactors, but I think this is a good direction.

@epage how does the basic functionality look to you? We can then put the traits suggested in #2 on top of this struct shape.

@epage
Copy link
Contributor

epage commented Apr 7, 2018

I'm a bit limited on time and the heavy use of macros is making it more difficult to see what you are intending with the API. Honestly, I wonder if a build.rs would serve you better than macros, or just letting keeping it written out by hand.

@CAD97
Copy link
Collaborator Author

CAD97 commented Apr 7, 2018

No bother. Actually, representing this as a DSL and transforming using a build step doesn't sound that bad.

The current implementation looks roughly like this:

#[derive(Clone, Debug)]
pub struct Circle {
    /// Your home directory.
    home: Option<PathBuf>,
    ...
    /// The name of the git tag being tested.
    tag: Option<String>,
    ...
}

impl Circle {
    pub fn lazy() -> Option<Self> {
        Circle {
            home: None,
            ...
            tag: None,
            ...
        }
    }

    pub fn eager() -> Self {
        let this = Self::lazy().unwrap();
        let _ = this.home();
        ...
        let _ = this.tag();
        ...
        this
    }
}

impl Circle {
    /// Your home directory.
    pub fn home(&mut self) -> &PathBuf {
        if self.home.is_none() {
            self.home = env("HOME")
                .map(|it| it.parse().unwrap());
        }
        self.home.as_ref().unwrap()
    }
    ...
    /// The name of the git tag being tested.
    pub fn tag(&mut self) -> Option<&String> {
        if self.tag.is_none() {
            self.tag = env("TAG")
                .map(|it| it.parse().unwrap());
        }
        self.tag.as_ref()
    }
    ...
}

Except with actual panic messages.

@epage
Copy link
Contributor

epage commented Apr 7, 2018

I assume the impl Circle fns are home(&mut self)? My concern is that requiring that much mutable state (mut for read operations) could cause problems with the borrow checker and isn't leveraging the capabilities of Rust.. The alternative is to directly access the member but then the user has to juggle the Options, again not leveraging the capabilities of Rust.

A very rough idea of where I lean on an API is:

  • state-less structs that support serde
  • data-only structs that can be loaded from serde
    It'd take seeing a prototype and playing with it to iterate on what the best way to handle things is.

This would allow lazy loading, loading into custom structs, and a default struct implementation that contains everything for the person who needs it. There might need to be some ergonomic improvements for that case, like a From<> or something.

@CAD97
Copy link
Collaborator Author

CAD97 commented Apr 7, 2018

You're correct, fixed the missing &mut self.

Another proposal: create a LazyEnv type. (very pseudo)

struct LazyEnv<T> {
    inner: Cell<Option<Option<T>>>
    var: &'static str
}
impl<T> LazyEnv<T> {
    pub fn get(&self) -> Option<&T> {
        self.inner
            .get_or_insert_with(|| env(self.var))
            .as_ref()
    }
}

And then the CI structs would have an abundance of those? Pending making a version for expected versions as well as the maybe-absent.

@CAD97
Copy link
Collaborator Author

CAD97 commented Apr 7, 2018

But I'll spend some time investigating building on top of serde deserialization. It's just that no two CI agree on what stuff is called.

@epage epage mentioned this pull request Jan 8, 2019
@CAD97
Copy link
Collaborator Author

CAD97 commented Jan 9, 2019

I'm closing this as 1) it's super stale and 2) the current interface is probably better short-term at least.

@CAD97 CAD97 closed this Jan 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants