Skip to content

Conversation

pukeko37
Copy link

* Responded to Issue #19 - added humidity module
* Created density module (required for one output from Humidity)
* Updated ReadMe to include Density and Humidity
* Added new modules into lib.rs
* [TODO: updates to CHANGELOG, etc...]
  [TODO (Possibly): Add more native units (g/m3), (kg/L), (g/mL), (t/m3), (oz/cu in), (oz/US fl oz), (lb/cu yd)... ]

    * Responded to Issue #19 - added humidity module
    * Created density module (required for one output from Humidity)
    * Updated ReadMe to include Density and Humidity
    * Added new modules into lib.rs
    * [TODO: updates to CHANGELOG, etc...]
      [TODO (Possibly): Add more native units (g/m3), (kg/L), (g/mL), (t/m3), (oz/cu in), (oz/US fl oz), (lb/cu yd)... ]
@drodil
Copy link

drodil commented Oct 12, 2020

@eldruin should this also be reviewed? It has been here quite a long time..

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

This is great stuff @pukeko37!
I just added a couple nitpicks and questions (I'm no native English speaker).
Could you also add an entry to the changelog?

Thanks @drodil for highlighting it! Maybe you can add a from_str for it soon ;)

@jhbruhn
Copy link

jhbruhn commented May 11, 2021

How are the chances of this getting merged? I am implementing a sensor driver which would need the temperature and humidity structs and would prefer to use this library for both!

pukeko37 and others added 3 commits June 2, 2021 17:07
@pukeko37
Copy link
Author

pukeko37 commented Jun 2, 2021

Sorry about the delay. I submitted the PR over a year ago and got no response for a long time. It was supposed to be a quick PR to get changes I needed upstream, so I could use the crate in my app. I didn't even put my changes in a new feature branch. In the end, because I got no response from the maintainers, I put my changes in my own private library. So, I lost interest in helping ...

Anyway. I've responded to the maintainers. Up to them now.

Added changelog entry
Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Sorry about initially not getting a response. This project has now been transferred to an umbrella org.
The changes look good to me. Just a nitpick about the changelog left and it would be ready for merge.

Co-authored-by: Diego Barrios Romero <[email protected]>
Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@eldruin
Copy link
Member

eldruin commented Jun 2, 2021

@pukeko37 could you rebase this against master? GitHub is complaining about conflicts.

@pukeko37
Copy link
Author

pukeko37 commented Jun 2, 2021

I just noticed, The README.md has a list of modules in it. Not updated with humidity and density.

@pukeko37
Copy link
Author

pukeko37 commented Jun 2, 2021

I thought I did rebase it. I've been editing in GitHub. I don't have a local dev environment.

@pukeko37
Copy link
Author

pukeko37 commented Jun 2, 2021

I think that's done it, but I'm not sure. Please let me know

@eldruin
Copy link
Member

eldruin commented Jun 2, 2021

GitHub still shows conflicts. I'm not sure which, though.

@pukeko37
Copy link
Author

pukeko37 commented Jun 2, 2021

When I try to fetch upstream, my fork repository says: This branch is not behind the upstream rust-embedded-community:master.

Not sure what I need to do.

@pukeko37
Copy link
Author

pukeko37 commented Jun 2, 2021

This PR also says: "This branch has no conflicts with the base branch".

@eldruin
Copy link
Member

eldruin commented Jun 2, 2021

Ok I was able to rebase and merge these changes manually to the current master.
Thank you very much @pukeko37!

@eldruin eldruin closed this Jun 2, 2021
This was referenced Jun 2, 2021
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.

4 participants