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

Replace use of Any by an enum #21

Merged
merged 1 commit into from
Oct 9, 2019

Conversation

pcpthm
Copy link
Contributor

@pcpthm pcpthm commented Oct 9, 2019

Fixes #17.
I came from hacktoberfest tag and not familiar with the codebase so I kept in mind to make minimal changes.

Copy link
Collaborator

@Empty2k12 Empty2k12 left a comment

Choose a reason for hiding this comment

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

Looking good. Documentation can be improved for the enum, so users of this library know what it's doing.

@@ -50,6 +50,23 @@ impl fmt::Display for Timestamp {
}
}

pub enum InfluxDbQueryTypes<'a> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add some rustdoc comments so it's clear what this is used for for users of this library.

Copy link
Collaborator

@Empty2k12 Empty2k12 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for this contribution. Happy hacking for Hacktoberfest.

@Empty2k12 Empty2k12 added Status: Merge when CI passes This PR has been reviewed and accepted for merge and removed Status: Pending Updates labels Oct 9, 2019
@Empty2k12 Empty2k12 merged commit 8cc4b32 into influxdb-rs:master Oct 9, 2019
@pcpthm pcpthm deleted the remove-uses-of-any branch October 9, 2019 12:15
@Drevoed
Copy link

Drevoed commented Oct 9, 2019

Was this merge and Pull Request really necessary? We don't have to store references to read/write queries and more importantly, influxdb-rust already had QueryType. Code using Any could be refactored without the addition of redundant enums. Trait InfluxDbQuery describes a method get_type which returns either QueryType::Read/WriteQuery depending on a struct.

@Drevoed
Copy link

Drevoed commented Oct 9, 2019

@Empty2k12

    pub async fn query<Q>(&self, q: &Q) -> Result<String, InfluxDbError>
    where
        Q: InfluxDbQuery
    {
        let query = match q.build() {
            Err(err) => {
                let error = InfluxDbError::InvalidQueryError {
                    error: format!("{}", err),
                };
                return Err(error);
            }
            Ok(query) => query,
        };

        let basic_parameters: Vec<(String, String)> = self.into();

        let client = match q.get_type() {
            QueryType::ReadQuery =>
//...

this is how it could and should (imho) be done (why add something to the crate when we already have it) and is how I'm implementing an async-await unstable feature right now

@pcpthm
Copy link
Contributor Author

pcpthm commented Oct 9, 2019

@Drevoed If I can break API, my opinion is:

  • InfluxDbQuery trait is unfit because it is not semantically open (it has to be either read or write query, no consuming crate should be implement this trait). It is currently also used for something like a something like a module but is is wrong. Remove it.
  • New InfluxDbQueryTypes can be used only for the purpose of "overloading" emulation. Not necessary TBH.

@Drevoed
Copy link

Drevoed commented Oct 9, 2019

@pcpthm I agree and I've come up with a different solution then, seeing as crate is still in 0.0.* and allows breaking changes. Something along the lines of this:

pub enum InfluxDbQuery {
    Write(InfluxDbWriteQuery),
    Read(InfluxDbReadQuery)
}

impl InfluxDbQuery {
    pub fn build(&self) -> Result<ValidQuery, InfluxDbError> {
        use InfluxDbQuery::*;
        
        match self {
            Write(write_query) => write_query.build(),
            Read(read_query) => read_query.build()
        }
    }
}

this follows your idea of removing an InfluxDbQuery

@Empty2k12
Copy link
Collaborator

I'm happy to discuss a more semantic API for this crate. Usability and code readability for implementers should be highest concern. I had no one to check the semantics of the API I designed, so I'm thankful for all feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove uses of Any
3 participants