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

Feature/upload result manually #258

Merged
merged 61 commits into from Apr 21, 2019
Merged

Conversation

lorenzoPrimi
Copy link
Contributor

@lorenzoPrimi lorenzoPrimi commented Apr 6, 2019

Fixes ooni/probe#838
TestSummaryTableViewController.m became TestSummaryViewController.m but for some reason git think is a new file

@lorenzoPrimi lorenzoPrimi changed the base branch from master to rename_classes April 17, 2019 10:53
@lorenzoPrimi lorenzoPrimi changed the base branch from rename_classes to master April 17, 2019 10:53
Copy link
Member

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

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

It seems I never finished this review


@interface UploadFooterViewController ()

@end
Copy link
Member

Choose a reason for hiding this comment

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

Is this empty interface needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the xcode generated file. I never touch it.

Copy link
Member

Choose a reason for hiding this comment

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

I feel brave today :-). Can you please remove it? I'm sure I've written ObjectiveC code where this internal interface was not there, so we can safely remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing it still builds the project.
The fact is this interface is in basically every file so maybe is better to have a PR with a general cleanup rather than doing it only once in one file

Copy link
Member

Choose a reason for hiding this comment

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

Let's start with not increasing the entropy in this PR. Refactoring other files can be done incrementally.

("Always leave the camp cleaner than you find it.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so do you think this can be refactored here or in a next PR?

Copy link
Member

Choose a reason for hiding this comment

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

Just remove the three lines in this file, so we don't ever commit them. We'll deal with the rest with time and calm as there is no rush to convert all the code base today, just the desired to make this PR minimal.

@bassosimone
Copy link
Member

@lorenzoPrimi please, address the remaining comments. Then, I'll do a final pass.

Copy link
Member

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

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

🐳

@lorenzoPrimi lorenzoPrimi merged commit 9832119 into master Apr 21, 2019
@lorenzoPrimi lorenzoPrimi deleted the feature/upload_result_manually branch April 21, 2019 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants