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 spelling_language property #41

Merged
merged 2 commits into from
Feb 2, 2024
Merged

Conversation

cxw42
Copy link
Member

@cxw42 cxw42 commented May 29, 2023

Resolves editorconfig/editorconfig#315 .
To specify the natural language for which spelling should be checked.

Open questions:

  • Should we use RFC 5646 tags instead of xx_YY (ISO/IEC 15897) tags? The RFC is freely available, which the ISO standard is not.
  • If we do use ISO tags, should we restrict values at all? E.g., only alpha-2? Or expressly specify that alpha-3 is also accepted?

@mcepl
Copy link

mcepl commented May 29, 2023

I am not sure what exactly is the difference between RFC 5646 and ISO/IEC 15897, but my intent was to use the same string as with the Unix locale. Which apparently uses the ISO two-letter code. Is there any reason why would anybody want the three-letter one?

If the encoding from the locale string would be used (which I don’t expect, just to be on the nice side of the Postel’s Law), it would be silently removed.

@seifferth
Copy link

Is there any particular reason why you state that any charset in the spell_language setting will be silently ignored? I would much rather explicitly forbid adding a charset to that parameter. If the spec explicitly stated that no charset must be included in the spell_language setting, that should lead to more straightforward .editorconfig files as it completely removes the question of which of the two charsets will be used. Also, if the charset is forbidden in spell_language, as a plugin developer I would be able to ask people to fix their .editorconfig files. I wouldn't be able to do that if a second but ignored charset is included in the actual spec.

@mcepl
Copy link

mcepl commented May 29, 2023

Is there any particular reason why you state that any charset in the spell_language setting will be silently ignored?

Just The Postel’s Law.

@seifferth
Copy link

seifferth commented May 29, 2023 via email

@mcepl
Copy link

mcepl commented May 29, 2023

You are right. So, encoding is out of the spec.

@treyhunner
Copy link
Member

I agree with @mcepl and @seifferth's conclusions that the option shouldn't include a character set.

I'm just catching up on this whole long-time discussion and added a slight bit of bike-shedding (regarding the name) to the original issue. I think the name is important and I'm not sold on the current one.

Thanks for your work on this @cxw42! I love this idea and appreciate all the work that's been put into the discussion that @mcepl originally started.

@cxw42
Copy link
Member Author

cxw42 commented Jun 5, 2023

Edits:

@cxw42 cxw42 marked this pull request as ready for review July 1, 2023 17:08
@cxw42 cxw42 changed the title Add spell_language property Add spelling_language property Jul 15, 2023
seifferth added a commit to seifferth/vis-editorconfig that referenced this pull request Sep 25, 2023
Discussion on the editorconfig standard seems to converge
on calling the property 'spelling_language' rather than
'spell_language' (see [1]). This commit adjusts both the
value that is expected to be found in .editorconfig files
as well as the internal variable that stores this property.

Note to editorconfig users: This commit changes the plugin's
behaviour in a non backwards compatible way. All editorconfig
files that contain the 'spell_language' property need to be
updated.

Note to spellchecker plugin developers: Since this commit
also updates the internal variable name, plugins need to
update this name as well.

[1] editorconfig/specification#41
@cxw42
Copy link
Member Author

cxw42 commented Nov 6, 2023

@editorconfig/board-member Would some of you be willing to review? Is this OK to merge into the spec? Thanks!

@ffes
Copy link
Member

ffes commented Nov 6, 2023

What still isn't determined in the discussion in editorconfig/editorconfig#315 is the problem of multiple languages. Being Dutch (and therefore being affected by this personally 😉) I really think there should be at least a note about multiple languages. "Separated by commas" seems the best option to me, but "Not supported" can also be an option at this moment.

@ffes
Copy link
Member

ffes commented Nov 6, 2023

Another thing I noticed it that the values of charset have hyphens, the values of spelling_language have underscores. That is inconstant.

@mcepl
Copy link

mcepl commented Nov 7, 2023

Another thing I noticed it that the values of charset have hyphens, the values of spelling_language have underscores. That is inconstant.

Postel’s law: Be conservative in what you do, be liberal in what you accept from others

Correct form is with underscore (that's ISO language standard), but hyphen SHOULD be accepted as well.

@ffes
Copy link
Member

ffes commented Nov 7, 2023

We are defining the specs for EditorConfig, so we need to make sure the specs are as well defined and consistent as possible.

It goes back to one of the open questions @cxw42 asks:

Use RFC 5646 tags or use ISO tags?

I think we just need to refer to a freely available list of languages and countries (as @cxw42 already does in this PR) and choose a default separator. From a practical point of view there isn't much difference between en-gb and en_GB. Therefore I would suggest to use the hyphen simply because charset already uses hyphens for values and therefore use RFC 5646 tags.

Note that the specs already say:

Pair keys are case insensitive. All keys are lowercased after parsing.

And what software supports, is up to them. It is always nice if they follow Postel’s law, but I wouldn't hope too much that organisations like MS or JetBreans do this. MS doesn't even follow the specs at this moment in Visual Studio (I experience that too often).

@cxw42
Copy link
Member Author

cxw42 commented Nov 11, 2023

@ffes Thanks for the review! I:

  • left the ISO references in place. RFC5646 refers to the same sources for codes in the Language-Region format, so we are consistent with the RFC.
  • changed the separator to a hyphen per your suggestion.
  • added "Only one language can be specified" since the discussion about multiple values is ongoing.
@@ -226,9 +226,9 @@ and the supported values associated with them:
        control the character set. Use of ``utf-8-bom`` is discouraged.
    * - ``spelling_language``
      - Sets the natural language that should be used for spell checking.
-       There is no default value.
+       Only one language can be specified.  There is no default value.
 
-       The format is ``ss`` or ``ss_TT``, where ``ss`` is an `ISO 639`_
+       The format is ``ss`` or ``ss-TT``, where ``ss`` is an `ISO 639`_
        language code and ``TT`` is an `ISO 3166`_ territory identifier.
 
        **Note:** This property does **not** specify the charset to be used.

To specify the natural language for which spelling should be checked.
@cxw42
Copy link
Member Author

cxw42 commented Nov 11, 2023

I also had to add a .readthedocs.yaml to get the checks to pass. I used the sample from https://docs.readthedocs.io/en/latest/config-file/v2.html.

@ffes
Copy link
Member

ffes commented Nov 21, 2023

@cxw42 Looks good to me.

I'm not sure though what the next step is. There wasn't a vote yet for this property by @editorconfig/board-member in https://github.com/editorconfig/editorconfig-vote

But I'm not sure how to start a vote to get it officially accepted. If if it just creating an issue in that repo I will gladly do so.

My vote will be obvious, I like this property.

@xuhdev
Copy link
Member

xuhdev commented Dec 30, 2023

You can start by opening an issue at https://github.com/editorconfig/editorconfig-vote/issues

The template is ready and will prompt you for what to do.

@cxw42
Copy link
Member Author

cxw42 commented Jan 15, 2024

I took the liberty of opening at vote at editorconfig/editorconfig-vote#15 . I am not a board member so please feel free to close if that was not the correct procedure! I saw no one else had done it so thought I would :) .

@xuhdev The template says "add the correct label" but GitHub did not give me an option to do so.

@ffes
Copy link
Member

ffes commented Jan 16, 2024

I am not a board member...

I am a bit surprised you are not a board member

@Mpdreamz
Copy link
Member

Mpdreamz commented Jan 17, 2024

I am a bit surprised you are not a board member

We have no formal method of nomination as far as I am aware but @cxw42 has my vote 😸

I am not a board member so please feel free to close if that was not the correct procedure! I saw no one else had done it so thought I would :)

AFAIK

  • anyone can nominate to vote but only board members can comment in the repository.
  • an issue in the vote repository follows after the discussion on a PR or issue reaches a consensus and would be ready to be merged or implemented.
  • NO pr should be merged into editorconfig/specification without a green lit vote.

In the past votes have been green lit both through quorum or through non-votes in absentia. I am unsure if we have hard rules here or even a minimum of votes.

@editorconfig/board-member please correct me!

We also have quite a few votes that got a yes but still open PR's/issues.
I will take a stab at labelling these.

</offtopic>

.readthedocs.yaml Show resolved Hide resolved
@treyhunner
Copy link
Member

treyhunner commented Jan 20, 2024

I am not a board member so please feel free to close if that was not the correct procedure! I saw no one else had done it so thought I would :) .

@cxw42 would you be comfortable with being nominated to join the board? You would have voting right for changes proposed to be made in EditorConfig (though I think you should abstain from editorconfig/editorconfig-vote#15 since you weren't a board member when voting started). If you are interested, we can start a vote to add you.

I'm asking because I second @ffes's surprise about you not being a board member. You've been very actively contributing to issues & discussions for years and I suspect you're more active than many current board members!

@cxw42
Copy link
Member Author

cxw42 commented Jan 28, 2024

Board member

@ffes @treyhunner @Mpdreamz Thanks very much for the votes of confidence! I am glad to be able to help make people's lives better through this project :) .

I would be happy to serve on the board, but I unfortunately cannot guarantee timely responses due to ongoing family medical issues and work demands. Specifically, I don't think I can meet this criterion:

Members should be able to respond (at least a yes or no) to it within two weeks, at least for most of the time.

:( ref.

(Just so you're not worried, the medical issues are nothing life-threatening, but they do randomize my schedule.)

@ffes
Copy link
Member

ffes commented Feb 1, 2024

I took the liberty of opening at vote at editorconfig/editorconfig-vote#15

The voting has been closed. The result is that the property spelling_language is accepted by @editorconfig/board-member.

@cxw42 cxw42 merged commit 0b3ffbd into editorconfig:master Feb 2, 2024
1 check passed
@cxw42 cxw42 deleted the spell-language branch February 2, 2024 14:32
Alhadis added a commit to Alhadis/.files that referenced this pull request Apr 25, 2024
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.

equivalent of vim's spell and spelllang settings
7 participants