Skip to content
This repository has been archived by the owner on Feb 8, 2023. It is now read-only.

Fix decoding two encoded chars after each other #18

Closed
wants to merge 6 commits into from

Conversation

tholu
Copy link
Contributor

@tholu tholu commented Feb 25, 2019

Related issue: #14

@tholu tholu changed the title WIP: Fix decoding two encoded chars after each other Fix decoding two encoded chars after each other Feb 25, 2019
@FagnerMartinsBrack
Copy link
Member

Hi @tholu thank you very much for the effort you've put on this! I really appreciate it!

Just one thing, I can see a lot of code style changes, editor-specific file changes, and code changes in the same PR. It's hard to build a conversation this way usign Github, it becomes very noise and too much context switching. In the spirit to make it easy to review and focus on the priorities, is it possible to split this in multiple PRs handling each a single thing?

For example, in this order:

1 PR just with the minimum code to fix #14
1 PR to fix dependencies
1 PR for removing the Eclipse-specific files
1 PR to change the code style from tabs to spaces
n PRs to do any other refactoring you want to introduce

This way we can talk about each change and I can still be aware of the codebase without getting distracted by a lot of changes all at once. I know this requires some work in your side, but I think it will be helpful to the project.

Is that ok? Thanks! again!

@tholu
Copy link
Contributor Author

tholu commented Feb 26, 2019

Thanks for your quick feedback! Some Codestyle changes were not really necessary, I‘m sorry for that! Out of a habit, I let IDEA do the default formatting.

As the project is quite out-of-shape, I needed to update the dependencies to get the tests running initially (and also consequently fix Travis, which is currently failing).

I will try to separate the changes as much as possible, and would start with the dependency cleanup and Travis fix, so the tests are running again. Then the test and fix for #14. Then any other changes.

@FagnerMartinsBrack
Copy link
Member

would start with the dependency cleanup and Travis fix, so the tests are running again

Awesome! I agree with that. Making it work, then make it better ;D

@tholu
Copy link
Contributor Author

tholu commented Feb 26, 2019

@FagnerMartinsBrack First splitted PR is in #19 - this should get the Travis build fixed and current tests running.

@tholu
Copy link
Contributor Author

tholu commented Feb 28, 2019

Closing in favor of #21.

@tholu tholu closed this Feb 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants