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

Rename pbs_light.go To main.go #1074

Merged
merged 1 commit into from
Oct 16, 2019
Merged

Conversation

SyntaxNode
Copy link
Contributor

It is common for the main func to be contained within main.go. I understand the pbs_light name is an artifact of an earlier phase of the project. I suggest renaming the app entry point for clarity.

glog.Errorf("prebid-server failed: %v", err)
}
}

func loadConfig() (*config.Configuration, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the Viper setup logic into its own method to mirror the encapsulation of the server setup. IMHO, this seems easier to read.

if err := serve(Rev, cfg); err != nil {

err = serve(Rev, cfg)
if err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The call to loadConfig cannot be inlined within the if block since the cfg is needed outside the block. Changing the usage of the server call to match for (hopefully you'll agree) improved readability.

func serve(revision string, cfg *config.Configuration) error {
currencyConverter := currencies.NewRateConverter(&http.Client{}, cfg.CurrencyConverter.FetchURL, time.Duration(cfg.CurrencyConverter.FetchIntervalSeconds)*time.Second)
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 line was quite long. Pulled out the time conversion.

Copy link
Contributor

@benjaminch benjaminch left a comment

Choose a reason for hiding this comment

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

Nice :)

Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

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

LGTM

@SyntaxNode SyntaxNode merged commit 5db4686 into prebid:master Oct 16, 2019
@SyntaxNode SyntaxNode deleted the rename-main branch October 16, 2019 19:23
mansinahar pushed a commit to mansinahar/prebid-server that referenced this pull request Nov 1, 2019
katsuo5 pushed a commit to flux-dev-team/prebid-server-1 that referenced this pull request Dec 1, 2020
katsuo5 pushed a commit to flux-dev-team/prebid-server-1 that referenced this pull request Dec 2, 2020
katsuo5 pushed a commit to flux-dev-team/prebid-server-1 that referenced this pull request Dec 4, 2020
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.

4 participants