Skip to content

Conversation

arnavb
Copy link

@arnavb arnavb commented Dec 19, 2017

The title says it all.

Copy link
Collaborator

@matthijskooijman matthijskooijman left a comment

Choose a reason for hiding this comment

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

Agreed, there's absolutely no reason to specify this explicitly. This should not affect the generated code in any way, so it seems safe to merge.

This change probably also needs to be applied to the SAM and SAMD cores, though.

@arnavb
Copy link
Author

arnavb commented Dec 19, 2017

Ok, should I make the changes on those files in this PR? (I can't seem to find the files, if you want me to make the changes, tell me where)

@matthijskooijman
Copy link
Collaborator

The other cores live in different repositories, so they'd need different PRs.

@facchinm, @cmaglie, what's the plan for these shared files? I remember some plans to extract them into a shared space, but I'm not sure how far along those plans are? Regardless, what's the best course to take here? Preparing separate PRs, or just copying over the entire Stream.cpp file at some later point?

@facchinm facchinm added Print and Stream class The Arduino core library's Print and Stream classes feature request A request to make an enhancement (not a bug fix) labels Dec 20, 2017
@arnavb
Copy link
Author

arnavb commented Feb 26, 2018

@matthijskooijman You said this was safe to merge a few months ago. Any chance of this actually happening? This is a minor change, so it shouldn't affect much. (Sorry to pester you)

@matthijskooijman
Copy link
Collaborator

@arnavb, I'm not in a position to merge things, I usually only review stuff. You'll have to pester @cmaglie and @facchinm instead :-p

@arnavb
Copy link
Author

arnavb commented Feb 26, 2018

Well, since you've pinged them, that's good.

@facchinm
Copy link
Member

facchinm commented Mar 2, 2018

Hi @arnavb , sure, we are in the middle of a transition to https://github.com/arduino/ArduinoCore-avr so I'd prefer not to merge the PR here but to merge it there. Furthermore, as you can see from the "Print and Stream class" tag, it is not an hardware related change so it's going to be merged as part of Chainsaw project. Sorry for the confusion about all these repos but we are trying to provide on a more modern way to contribute to the project.

@arnavb
Copy link
Author

arnavb commented Mar 2, 2018

@facchinm Makes sense! Thanks for the info!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request A request to make an enhancement (not a bug fix) Print and Stream class The Arduino core library's Print and Stream classes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants