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 to reduce package count #83

Merged
merged 10 commits into from
Dec 4, 2019
Merged

Refactoring to reduce package count #83

merged 10 commits into from
Dec 4, 2019

Conversation

hellais
Copy link
Member

@hellais hellais commented Dec 2, 2019

  • Consolidate util and utils into the same package
  • Move internal/onboard into internal/cli/onboard
  • Move maybeOnboard into the onboard package
  • Add code coverage checks

What I am looking for in a review is:

  1. If it seems like a good idea to go for this approach of reducing the package count and making the directory structure a bit more flat
  2. If anything pops out as odd or unusual in the refactoring

No new code has been added (beyond adding new tests and code coverage checks).

}
return nil
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This has just been moved into here from the separate onboarding package.

import (
"github.com/ooni/probe-cli/nettests"
)
package nettests
Copy link
Member Author

Choose a reason for hiding this comment

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

All nettests have been flattened into the root nettests package. I think eventually we should move all the nettest specific code into probe-engine as there is currently so many levels of indirection going on here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I believe we should aim for slowly moving code in probe-engine.

t.Fatal(err)
}
nt := HTTPInvalidRequestLine{}
ctl := NewController(nt, ctx, res)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a very basic smoke test.

Copy link
Contributor

@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.

As a general rule, I tend to put as much code as possible into the internal package so I am not surprising people when I do refactoring. This is something we could consider for the future.

For now, 🐳

import (
"github.com/ooni/probe-cli/nettests"
)
package nettests
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I believe we should aim for slowly moving code in probe-engine.

@hellais hellais merged commit 68d00c8 into master Dec 4, 2019
@hellais hellais deleted the refactor-consolidate branch December 4, 2019 09:00
ainghazal pushed a commit to ainghazal/probe-cli that referenced this pull request Mar 8, 2022
Refactoring to reduce package count
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants