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

Add a codespell lint job #2395

Merged
merged 2 commits into from
Jun 28, 2024
Merged

Add a codespell lint job #2395

merged 2 commits into from
Jun 28, 2024

Conversation

matrss
Copy link
Collaborator

@matrss matrss commented May 24, 2024

Purpose of PR?:

Fixes #2329.

Does this PR introduce a breaking change?

If the changes in this PR are manually verified, list down the scenarios covered::

Additional information for reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Does this PR results in some Documentation changes?
If yes, include the list of Documentation changes

Checklist:

  • Bug fix. Fixes #
  • New feature (Non-API breaking changes that adds functionality)
  • PR Title follows the convention of <type>: <subject>
  • Commit has unit tests

@matrss matrss changed the base branch from develop to stable May 24, 2024 15:33
@matrss matrss force-pushed the add-codespell branch 5 times, most recently from 179de4c to 79eae63 Compare May 27, 2024 15:42
@matrss matrss marked this pull request as ready for review May 27, 2024 16:05
@matrss matrss requested a review from ReimarBauer May 27, 2024 16:05
@ReimarBauer
Copy link
Member

seems I overlooked this. lets see if we can add it to 9.0.1

docs/development.rst Outdated Show resolved Hide resolved
@@ -128,7 +128,7 @@ def _plot_style(self):


def make_msschem_hs_class(
entity, nam, vert, units, scale, add_data=None, add_contours=None, fix_styles=None,
entity, name, vert, units, scale, add_data=None, add_contours=None, fix_styles=None,
Copy link
Member

Choose a reason for hiding this comment

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

this qualifies for going to a 9.1.0 which is also fine

@@ -30,13 +30,13 @@
from PyQt5 import QtCore, QtWidgets

from mslib.utils import FatalUserError
from mslib.msui import aircrafts, constants
from mslib.msui import aircraft, constants
Copy link
Member

@ReimarBauer ReimarBauer Jun 26, 2024

Choose a reason for hiding this comment

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

that's an API change. The issue the work is based on was added for 10.0.0.

To get the valuable other changes in could you undo the API renamings and move this work to an issue e.g. improve names for the next major (10.0.0)?

The name maybe was also choosen for adding more than only the SimpleAircraft class. We maybe just have not added more, e.g. ressources https://www.littlenavmap.org/downloads/Aircraft%20Performance/MSFS/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The name maybe was also choosen for adding more than only the SimpleAircraft class.

The plural of "aircraft" is "aircraft", not "aircrafts". There is nothing wrong with adding more Aircraft classes in there.

that's an API change. The issue the work is based on was added for 10.0.0.

Is the modules name part of an external API? How is it used from outside of the MSS codebase?

Copy link
Member

Choose a reason for hiding this comment

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

that's a good question. I don't know how people use our modules.

Semantic versioning is all about communicating the nature and scope of the changes to the code through the changes to the version number.

I don't want to do that differently.

Copy link
Member

Choose a reason for hiding this comment

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

The plural of "aircraft" is "aircraft", not "aircrafts". There is nothing wrong with adding more Aircraft classes in there.

interesting, it is defining the family of helicopter, balloon, airplane ...

Copy link
Member

@ReimarBauer ReimarBauer Jun 26, 2024

Choose a reason for hiding this comment

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

We don't warn users writing plugins about parts we still see as unstable. Some parts were in the past flagged as experimental. That module was introduced 8 year ago. It rather won't have that category.
If someone wants to add to an output plugin performance data it can be done.

Usually users ask only if they need help. So it is difficult to see what they have done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Semantic versioning is all about communicating the nature and scope of the changes to the code through the changes to the version number.

Semantic versioning is about communicating the nature and scope of the changes to the public API of the code through the changes to the version number. What happens internally doesn't matter, unless it affects the behavior of the public API.

Unfortunately, python is ill-equipped to make this distinction (other than the leading underscore convention), so one could argue that every detail is automatically part of the public API.

Anyway, I can move that particular change to a different PR to develop.

Copy link
Member

@ReimarBauer ReimarBauer left a comment

Choose a reason for hiding this comment

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

with the API change of renaming aircrafts we can only add this to stable when we afterwards release 10.0.0.

Because we currently have only one model in that module does not qualify the name change. I would keep the name or we have to change it again, when we add more aircraft models.

The text changes and parameter changes of nam to name qualifies all other changes to a "MINOR version when you add functionality in a backwards-compatible manner"
So we could rename the milestone to 9.1.0 and merge it.

Copy link
Member

@ReimarBauer ReimarBauer left a comment

Choose a reason for hiding this comment

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

boah, what a lot of typos we have corrected - and they won't come back :)

@ReimarBauer ReimarBauer merged commit e7af5dc into Open-MSS:stable Jun 28, 2024
11 checks passed
@matrss matrss deleted the add-codespell branch June 28, 2024 14:48
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.

Add linting with codespell
2 participants