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

Refactoring #199

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Refactoring #199

wants to merge 9 commits into from

Conversation

PointlessUser
Copy link

No description provided.

@PointlessUser PointlessUser requested review from a team as code owners October 7, 2023 04:42
@PointlessUser
Copy link
Author

PointlessUser commented Oct 7, 2023

Black Formatter caused a lot of the line changes. The other 4 commits are the ones that need review

Copy link
Contributor

@rcunrau rcunrau left a comment

Choose a reason for hiding this comment

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

If there's something other than auto refactoring you want looked at you should put it into a separate PR.

@PointlessUser
Copy link
Author

I reverted the auto-refactoring

@rcunrau
Copy link
Contributor

rcunrau commented Oct 8, 2023

I see the reversion commits, and yet there are still 26 files changed, and all the commit titles still include "refactor". Did you do all that refactoring by hand? (Impressive :-) Did you test your changes in any way? Normally, a refactor doesn't need (as much) testing because the changes aren't intended to be semantic, but for an interpreted language you can't use "see, it compiles" as proof you didn't break anything.

@PointlessUser
Copy link
Author

I'm not sure how to test it. The instructions on the readme don't seem to work

@rcunrau
Copy link
Contributor

rcunrau commented Oct 21, 2023

There are a bunch of different configurations, depending on whether you are connecting to a dev board or flat sat via UHF or USB. We can meet next week to try it out (although it's a busy week with MCR etc).

@rcunrau
Copy link
Contributor

rcunrau commented Oct 21, 2023

It says you merged 'master' into 'Refactoring', which, unfortunately, is backwards :-/
We actually want to merge 'Refactoring' into 'master' (as per the Merge buttons below) once we have tested that we haven't regressed anything. Did you do 'git pull' into your branch? You might be able to undo it with 'git unpull', but that doesn't usually work. The command you want if your branch is out of date is "git pull --rebase' to place your changes on top of the lastest master.

@PointlessUser
Copy link
Author

If you're busy this week, we could test it next week.

Also I merged main into refactoring so that the code is up to date with the main branch. There was a merge conflict I wanted to resolve and it makes merging back into main a lot easier

@rcunrau
Copy link
Contributor

rcunrau commented Oct 22, 2023

It's true you can't merge your branch if there are conflicts. Unfortunately, merging main into the branch messes up the history. Fortunately, we are unlikely to have to use the history to find any regressions. We're trying to follow the same processes you're going to have to follow when you get to industry. I can help you resolve the conflicts if it happens again.

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