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.

Sign up for GitHub

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

Jump to bottom

Strings management: use the newest recommended way to handle literals #443

Merged
merged 1 commit into from
May 28, 2019
Merged

Strings management: use the newest recommended way to handle literals #443

merged 1 commit into from
May 28, 2019

Conversation

s-pace
Copy link
Contributor

@s-pace s-pace commented Apr 12, 2019 •
edited
Loading

Porting to python 3 has been done thanks to several small PRs

This PR is now enhancing the string management.

@s-pace s-pace requested a review from BenoitPerrot April 12, 2019 17:12
redox
redox reviewed Apr 17, 2019
View reviewed changes
Copy link
Contributor

@redox redox left a comment

Choose a reason for hiding this comment

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

Great job! Good to see the port wasn't that hard.

Hugo-Rybinski
Hugo-Rybinski reviewed Apr 17, 2019
View reviewed changes
Copy link

@Hugo-Rybinski Hugo-Rybinski left a comment

Choose a reason for hiding this comment

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

  • You should use a linter (like flake8)
  • Type hints with a type linter (like mypy) is a nice improvement for code readability

README.md Outdated Show resolved Hide resolved
cli/src/commands/bootstrap_config.py Outdated Show resolved Hide resolved
cli/src/commands/bootstrap_config.py Outdated Show resolved Hide resolved
deployer/src/algolia_internal_api.py Outdated Show resolved Hide resolved
deployer/src/algolia_internal_api.py Outdated Show resolved Hide resolved
scraper/src/config/config_loader.py Outdated Show resolved Hide resolved
scraper/src/config/config_loader.py Outdated Show resolved Hide resolved
scraper/src/strategies/default_strategy.py Outdated Show resolved Hide resolved
scraper/src/strategies/default_strategy.py Outdated Show resolved Hide resolved
scraper/src/strategies/default_strategy.py Outdated Show resolved Hide resolved
@s-pace s-pace mentioned this pull request Apr 17, 2019
Closed
s-pace
s-pace commented Apr 17, 2019
View reviewed changes
Copy link
Contributor Author

@s-pace s-pace left a comment

Choose a reason for hiding this comment

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

Using review. Thank you @Hugo-Rybinski

deployer/src/helpers.py Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
deployer/src/config_manager.py Outdated Show resolved Hide resolved
deployer/src/index.py Outdated Show resolved Hide resolved
deployer/src/snippeter.py Outdated Show resolved Hide resolved
scraper/src/config/browser_handler.py Outdated Show resolved Hide resolved
@s-pace s-pace force-pushed the porting_p3 branch 4 times, most recently from 11728aa to 538b940 Compare May 16, 2019 07:00
BenoitPerrot
BenoitPerrot requested changes May 16, 2019
View reviewed changes
Copy link
Contributor

@BenoitPerrot BenoitPerrot left a comment

Choose a reason for hiding this comment

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

Please see inline comments

scraper/src/strategies/default_strategy.py Outdated Show resolved Hide resolved
deployer/src/algolia_internal_api.py Outdated Show resolved Hide resolved
deployer/src/algolia_internal_api.py Outdated Show resolved Hide resolved
deployer/src/helpers.py Outdated Show resolved Hide resolved
BenoitPerrot
BenoitPerrot requested changes May 20, 2019
View reviewed changes
deployer/src/helpers.py Outdated Show resolved Hide resolved
deployer/src/algolia_internal_api.py Outdated Show resolved Hide resolved
deployer/src/algolia_internal_api.py Outdated Show resolved Hide resolved
scraper/src/strategies/default_strategy.py Outdated Show resolved Hide resolved
@s-pace s-pace force-pushed the porting_p3 branch 3 times, most recently from 020ef9b to 529a492 Compare May 28, 2019 07:52
@s-pace s-pace changed the title Python 3 Porting Strings management: use the newest recommended way to handle literals May 28, 2019
@s-pace s-pace merged commit aeca859 into master May 28, 2019
@s-pace s-pace deleted the porting_p3 branch May 28, 2019 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hugo-Rybinski Hugo-Rybinski Hugo-Rybinski left review comments

@redox redox redox left review comments

@BenoitPerrot BenoitPerrot BenoitPerrot approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@s-pace @redox @BenoitPerrot @Hugo-Rybinski