Skip to content

Conversation

@Keeper-of-the-Keys
Copy link

As discussed by mail I'm going to do piece meal PRs for the FTDI stuff.

Kicking it off with a few logistical parts of the big PR:

  • .gitignore
  • travis lint blacklist of autogenerated files
  • Fix of escaping for README files into C headers.

@peternewman peternewman added this to the 0.11.0 milestone Oct 29, 2019
Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

Some comments.

You could add a bit more code into the review if you wanted, like some of the timing changes?

@peternewman peternewman self-assigned this Oct 29, 2019
Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

A few more minor comments, partly not actually your code sorry.

Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

Fix the typo

@peternewman
Copy link
Member

Please can you fix the issues codespell caught, I appreciate they aren't yours (sorry). I'm genuinely not sure if the OutputStream one should be struct-based or structured, I'm beginning to suspect the latter.

https://travis-ci.org/OpenLightingProject/ola/jobs/605241223#L1061-L1062

@Keeper-of-the-Keys
Copy link
Author

@peternewman Is anything still blocking this (the compile failures are I assume due to the race conditions since there is no reason to assume compilation would be affected by a commit that changes no code)?

@peternewman
Copy link
Member

@peternewman Is anything still blocking this (the compile failures are I assume due to the race conditions since there is no reason to assume compilation would be affected by a commit that changes no code)?

I'm hoping to fix the Linux builds at least in #1589 just to check this passes that, it's probably a bit belt and braces.

@Keeper-of-the-Keys
Copy link
Author

So nothing is blocking this from being merged anymore?

Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

Blocking on #1589 to ensure at least some builds complete with these changes, but the changes LGTM.

@peternewman
Copy link
Member

So nothing is blocking this from being merged anymore?

Correct, well not of yours anyway.

E.S. Rosenberg a.k.a. Keeper of the Keys added 2 commits November 10, 2019 02:31
@Keeper-of-the-Keys
Copy link
Author

Keeper-of-the-Keys commented Nov 10, 2019

@peternewman I took the liberty of purging .travis.yml of all legacy tests to get an idea if this would lead to more successful CI builds and also speed up things...

If you oppose I will push another commit reinstating these checks however as I stated in #1589 I believe that gcc-8 should not be part of legacy tests since it is not part of their toolchains, if we want to test gcc-8 we can either add bionic tests to the existing matrix or we can say that "hey master deals with something that will be released and has no relevance on Ubuntu Xenial or OS X from 3 years ago lets treat it that way"

@peternewman
Copy link
Member

@peternewman I took the liberty of purging .travis.yml of all legacy tests to get an idea if this would lead to more successful CI builds and also speed up things...

If you oppose I will push another commit reinstating these checks however as I stated in #1589 I believe that gcc-8 should not be part of legacy tests since it is not part of their toolchains, if we want to test gcc-8 we can either add bionic tests to the existing matrix or we can say that "hey master deals with something that will be released and has no relevance on Ubuntu Xenial or OS X from 3 years ago lets treat it that way"

Can you revert them for now (at least in this PR, as it's essentially ready to be merged). I'll try and find some time soon to respond more fully to your comments in the other PR, although an issue might be a more effective place to do it, currently the discussion is scattered around a number of PRs and other places (probably some on the mailing list too).

@peternewman
Copy link
Member

@Keeper-of-the-Keys if you can revert the last three commits and either pull in the changes from #1590 or wait for @nomis52 to approve them and me to merge them then we can merge this in.

@Keeper-of-the-Keys
Copy link
Author

Done and done

Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

Can you remove some errant merge stuff please.

@Keeper-of-the-Keys
Copy link
Author

Whoops 1 second forgot why I was explicitly defining files instead of commit -a because I had that debug stuff and hadn't thrown it out yet.

@Keeper-of-the-Keys
Copy link
Author

Done

@Keeper-of-the-Keys
Copy link
Author

@peternewman the functionality you requested was not present as is and would have actually broken the C++ string...
I now added it for Linux but have no way to check it for OS X.

E.S. Rosenberg a.k.a. Keeper of the Keys and others added 2 commits November 10, 2019 16:35
E.S. Rosenberg a.k.a. Keeper of the Keys added 2 commits November 11, 2019 01:55
@Keeper-of-the-Keys
Copy link
Author

Keeper-of-the-Keys commented Nov 11, 2019

reverted.
I love how it needs to be tested on every OS out there but Linux.... (because until I pointed out that on Linux it's not inserting newlines no one knew or cared)

@Keeper-of-the-Keys
Copy link
Author

See #1593 for the other improvement.

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