Skip to content

review#3

Open
noamzaks wants to merge 1 commit intomainfrom
code-review
Open

review#3
noamzaks wants to merge 1 commit intomainfrom
code-review

Conversation

@noamzaks
Copy link
Copy Markdown
Member

No description provided.

@noamzaks noamzaks requested a review from liorsagy August 31, 2022 15:40
Comment thread src/tasks/buildLocal.ts

if (!CMakeLists.includes("#local")) {
if (!CMakeLists.includes("#local")) { // What is this? Why does CMakeLists include #local?
// I'm guessing it's because you are lazy and you didn't use an `if` in the CMakeLists.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Its because i want to know if i need to delete the things in cbuild

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Why would you ever need to delete them?

It's dumb.
Have two different build directories for roborio and local.

I suggest build/local and build/roborio

Comment thread src/tasks/buildLocal.ts
"Build Local",
"vscode-miscar",
new vscode.ShellExecution(
// Unless you are a complete dumbass, you know this doesn't work on other OSs.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I will think about other solution for os

Comment thread src/tasks/deploy.ts
const deploy = async (context: vscode.ExtensionContext) => {
// I'm not checking this.
// It's way to similar to the fast deploy.
// You gotta extract the shared code into a function
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I will do it

Comment thread src/tasks/fastDeploy.ts
]
let connects: any[] = []
adresses.map((adress) => {
adresses.map((adress) => { // Address is pronounced with a double d
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

At least i was close

Comment thread src/tasks/fastDeploy.ts
"172.22.11.2",
"roborio-1574-frc.local",
"roborio-1574-frc.local", // Try to generalize the team number.
// Learn about VSCode extension preferences,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I will try to do that

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