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

Fix misc. typos #489

Merged
merged 1 commit into from Sep 19, 2019
Merged

Fix misc. typos #489

merged 1 commit into from Sep 19, 2019

Conversation

luzpaz
Copy link
Contributor

@luzpaz luzpaz commented Feb 10, 2019

codespell -q 3 -S *.ts,./libs,./sample-projects -L searchin

@gedakc
Copy link
Collaborator

gedakc commented Feb 11, 2019

Thank you @luzpaz for your interest in Manuskript and your efforts to make it better.

This PR identifies a number of spelling mistakes to fix which is a good thing.

However some of the spelling changes involve automatically generated content, and one spelling change affects executable code. As such I have some suggestions for this Pull Request.

  1. Because the CHANGELOG.md file is automatically generated from the github project (see bottom of CHANGELOG.md), I do not know the implications of making manual changes. Will these manual changes be overwritten on the next automatic generation? Is it better to try to fix the spelling mistakes in the title of the original Issue or Pull Request?

    Would you please test automatic generation, or else remove all the changes to CHANGELOG.md in this PR.

  2. This PR identifies itself as fixing spelling mistakes, but also changes executable code (see misspelled "stlye" in manuskript/ui/highlighters/markdownHighlighter.py#L679.

    Please place this executable code change in a separate PR. Please also test to see what this change fixes and include this in the PR description / commit message.

@luzpaz
Copy link
Contributor Author

luzpaz commented Feb 11, 2019

I will follow-up when I have another moment. Thanks.

@worstje
Copy link
Contributor

worstje commented Sep 17, 2019

@luzpaz What is the status on this?

@luzpaz
Copy link
Contributor Author

luzpaz commented Sep 17, 2019

forgot about this. Will look in to it

luzpaz added a commit to luzpaz/manuskript that referenced this pull request Sep 17, 2019
@luzpaz luzpaz mentioned this pull request Sep 17, 2019
@luzpaz
Copy link
Contributor Author

luzpaz commented Sep 17, 2019

@worstje
Both 4ddab3d & fe5777b are the reverted code requested by @gedakc
b714864 are a few more typos I just found, that need review

@worstje
Copy link
Contributor

worstje commented Sep 17, 2019

Can you rebase your entire set of changes against the current develop branch?

I am not entirely sure happened here, but I think you left your Feb 10 changes in (8830ea1), pulled in new more recent changes from develop, and then wrote these new patches against your old patch, leading to the commits where you 'revert'.

Ideally your PR only has the 'Misc. typos' and 'Add a few more typos' fixes. (And even those can be squashed together into a single commit.

Edit: Nevermind. Github confused me with the old date. It seems like everything is properly on top of the current develop. All you need is a squash.

@luzpaz
Copy link
Contributor Author

luzpaz commented Sep 17, 2019

squashed and ready to go

@worstje
Copy link
Contributor

worstje commented Sep 17, 2019

I'd amend the commit message a bit still because some of that description doesn't make sense anymore now that you've tidied it up. Other than that, it looks good! 👍

Found via `codespell -q 3 -S *.ts,./libs,./sample-projects -L searchin`
@luzpaz luzpaz changed the title Misc. typos Fix misc. typos Sep 17, 2019
@luzpaz
Copy link
Contributor Author

luzpaz commented Sep 17, 2019

Yea, makes sense. Amended.

gedakc pushed a commit that referenced this pull request Sep 17, 2019
Split off from #489
@gedakc
Copy link
Collaborator

gedakc commented Sep 19, 2019

@luzpaz are you able to make the suggested "save" -> "saved" changes in the next day or so?

If not then I will plan to merge this as-is and make the change afterwards.

@gedakc gedakc added this to the 0.10.0 milestone Sep 19, 2019
@luzpaz
Copy link
Contributor Author

luzpaz commented Sep 19, 2019

@gedakc i don't follow? was there a suggestion that I missed ?

@gedakc
Copy link
Collaborator

gedakc commented Sep 19, 2019

This looks like my bad. I must have been reviewing an older commit in my comments above.

When pulled your changes into my local repo, I can clearly see that you did change "save" to "saved" in both the files:

  • manuskript/ui/settings_ui.py
  • manuskript/ui/settings_ui.ui

I will plan to merge this PR for inclusion in the next release of Manuskript.

Thank you for your help.

@gedakc gedakc merged commit 2fd45dc into olivierkes:develop Sep 19, 2019
luzpaz added a commit to luzpaz/manuskript that referenced this pull request Sep 19, 2019
@luzpaz
Copy link
Contributor Author

luzpaz commented Sep 19, 2019

@gedakc you're absolutely right, I did miss a fix. Apologies. Added in #648

@gedakc
Copy link
Collaborator

gedakc commented Sep 19, 2019

@luzpaz no worries. I just remembered that yesterday I had pulled down your other PR with the typo fixes and then had made the "save" -> "saved" change locally. But with all the other work I did yesterday I forgot.

I'm glad you double-checked. I will merge this with the develop branch after the current Travis CI processes complete.

gedakc pushed a commit that referenced this pull request Sep 19, 2019
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.

None yet

3 participants