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

Implement flake8-i18n #3741

Merged
merged 13 commits into from
Mar 27, 2023
2 changes: 2 additions & 0 deletions LICENSE
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,8 @@ are:
CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
"""

- flake8-i18n, licensed as "GNU General Public License v2 (GPLv2)".
Copy link
Contributor

@AA-Turner AA-Turner Mar 27, 2023

Choose a reason for hiding this comment

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

@charliermarsh -- I'm not certain on this but wanted to ask the question -- does including a GPL tool in Ruff mean that (all/part of) Ruff would be considered a derivative work for the purposes of GPL, and therefore that Ruff would need to be re-licenced (or this PR reverted or etc)? This is the first GPL tool to be integrated, and I wanted to flag the concern as early as possible. Thanks, Adam

Copy link
Member

Choose a reason for hiding this comment

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

Lemme look into it.

Copy link
Contributor Author

@leiserfg leiserfg Mar 27, 2023

Choose a reason for hiding this comment

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

There are already other parts including GPL3 (flake8-django for instance).

  • edit: well only that one, but I think GPL3 is even more infectious than GPL2.

Copy link
Member

Choose a reason for hiding this comment

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

I'll have to get some more informed opinions on this, I'm not sure myself on whether these constitute derivative works.

If they do, and we'd have to re-license under GPL, then I'd err on the side of removing them, or going back to the authors and asking if they'd consider re-licensing under MIT or a dual license.

flake8-django is confusing because on PyPI it appears dual-licensed, and it includes the MIT trove classifiers:

Screen Shot 2023-03-27 at 4 54 36 PM
Screen Shot 2023-03-27 at 4 55 30 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll have to get some more informed opinions on this, I'm not sure myself on whether these constitute derivative works.

Sorry to cause the trouble and thank you both for looking in to this.

flake8-django is confusing because on PyPI it appears dual-licensed, and it includes the MIT trove classifiers:

It seems this has been noted already: rocioar/flake8-django#101 and rocioar/flake8-django#102. Sorry not to have flagged this at the time for flake8-django, but it seems the MIT trove classifier may have been a mistake when that project adopted Poetry.

A

Copy link
Member

Choose a reason for hiding this comment

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

No, thank you for calling this out! It's important to get right. I'll try to reach out to the flake8-django maintainer.

Copy link

Choose a reason for hiding this comment

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

Hum... This is such a common "idea", I've done it before without even knowing about flake8-i18n. We were working in a project and switched to Python 3.6 and many members of a the team started to use f-strings inside _. I became such a common mistake we had to "invent" our checker.

I'm not a lawyer, but I don't think this can be gathered as derivative work.


- flake8-implicit-str-concat, licensed as follows:
"""
The MIT License (MIT)
Expand Down
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ quality tools, including:
- [flake8-eradicate](https://pypi.org/project/flake8-eradicate/)
- [flake8-errmsg](https://pypi.org/project/flake8-errmsg/)
- [flake8-executable](https://pypi.org/project/flake8-executable/)
- [flake8-i18n](https://pypi.org/project/flake8-i18n/)
- [flake8-implicit-str-concat](https://pypi.org/project/flake8-implicit-str-concat/)
- [flake8-import-conventions](https://github.com/joaopalmeiro/flake8-import-conventions)
- [flake8-logging-format](https://pypi.org/project/flake8-logging-format/)
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/resources/test/fixtures/flake8_i18n/INT001.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
_(f"{'value'}")
charliermarsh marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions crates/ruff/resources/test/fixtures/flake8_i18n/INT002.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
_("{}".format("line"))
1 change: 1 addition & 0 deletions crates/ruff/resources/test/fixtures/flake8_i18n/INT003.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
_("%s" % "line")
34 changes: 29 additions & 5 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,12 @@ use crate::registry::{AsRule, Rule};
use crate::rules::{
flake8_2020, flake8_annotations, flake8_bandit, flake8_blind_except, flake8_boolean_trap,
flake8_bugbear, flake8_builtins, flake8_comprehensions, flake8_datetimez, flake8_debugger,
flake8_django, flake8_errmsg, flake8_implicit_str_concat, flake8_import_conventions,
flake8_logging_format, flake8_pie, flake8_print, flake8_pyi, flake8_pytest_style, flake8_raise,
flake8_return, flake8_self, flake8_simplify, flake8_tidy_imports, flake8_type_checking,
flake8_unused_arguments, flake8_use_pathlib, mccabe, numpy, pandas_vet, pep8_naming,
pycodestyle, pydocstyle, pyflakes, pygrep_hooks, pylint, pyupgrade, ruff, tryceratops,
flake8_django, flake8_errmsg, flake8_i18n, flake8_implicit_str_concat,
flake8_import_conventions, flake8_logging_format, flake8_pie, flake8_print, flake8_pyi,
flake8_pytest_style, flake8_raise, flake8_return, flake8_self, flake8_simplify,
flake8_tidy_imports, flake8_type_checking, flake8_unused_arguments, flake8_use_pathlib, mccabe,
numpy, pandas_vet, pep8_naming, pycodestyle, pydocstyle, pyflakes, pygrep_hooks, pylint,
pyupgrade, ruff, tryceratops,
};
use crate::settings::types::PythonVersion;
use crate::settings::{flags, Settings};
Expand Down Expand Up @@ -2898,6 +2899,29 @@ where
}
}

// flake8-i18n
if self.settings.rules.any_enabled(&[
Rule::FStringInI18NFuncCall,
Rule::FormatInI18NFuncCall,
Rule::PrintfInI18NFuncCall,
]) && flake8_i18n::rules::is_i18n_func_call(
func,
&self.settings.flake8_i18n.functions_names,
) {
if self.settings.rules.enabled(Rule::FStringInI18NFuncCall) {
self.diagnostics
.extend(flake8_i18n::rules::f_string_in_i18n_func_call(args));
}
if self.settings.rules.enabled(Rule::FormatInI18NFuncCall) {
self.diagnostics
.extend(flake8_i18n::rules::format_in_i18n_func_call(args));
}
if self.settings.rules.enabled(Rule::PrintfInI18NFuncCall) {
self.diagnostics
.extend(flake8_i18n::rules::printf_in_i18n_func_call(args));
}
}

// flake8-simplify
if self
.settings
Expand Down
5 changes: 5 additions & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,11 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
(Flake8Return, "507") => Rule::SuperfluousElseContinue,
(Flake8Return, "508") => Rule::SuperfluousElseBreak,

// flake8-i18n
(Flake8I18N, "001") => Rule::FStringInI18NFuncCall,
(Flake8I18N, "002") => Rule::FormatInI18NFuncCall,
(Flake8I18N, "003") => Rule::PrintfInI18NFuncCall,

// flake8-implicit-str-concat
(Flake8ImplicitStrConcat, "001") => Rule::SingleLineImplicitStringConcatenation,
(Flake8ImplicitStrConcat, "002") => Rule::MultiLineImplicitStringConcatenation,
Expand Down
7 changes: 7 additions & 0 deletions crates/ruff/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -627,6 +627,10 @@ ruff_macros::register_rules!(
rules::flake8_raise::rules::UnnecessaryParenOnRaiseException,
// flake8-self
rules::flake8_self::rules::PrivateMemberAccess,
// flake8-i18n
rules::flake8_i18n::rules::FStringInI18NFuncCall,
rules::flake8_i18n::rules::FormatInI18NFuncCall,
rules::flake8_i18n::rules::PrintfInI18NFuncCall,
// numpy
rules::numpy::rules::NumpyDeprecatedTypeAlias,
rules::numpy::rules::NumpyLegacyRandom,
Expand Down Expand Up @@ -777,6 +781,9 @@ pub enum Linter {
/// [flake8-type-checking](https://pypi.org/project/flake8-type-checking/)
#[prefix = "TCH"]
Flake8TypeChecking,
/// [flake8-i18n](https://pypi.org/project/flake8-i18n/)
#[prefix = "INT"]
Flake8I18N,
/// [flake8-unused-arguments](https://pypi.org/project/flake8-unused-arguments/)
#[prefix = "ARG"]
Flake8UnusedArguments,
Expand Down
29 changes: 29 additions & 0 deletions crates/ruff/src/rules/flake8_i18n/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
//! Rules from [flake8-i18n](https://pypi.org/project/flake8-i18n/).
pub(crate) mod rules;
pub mod settings;

#[cfg(test)]
mod tests {
use std::path::Path;

use anyhow::Result;
use insta::assert_yaml_snapshot;
use test_case::test_case;

use crate::registry::Rule;
use crate::settings;
use crate::test::test_path;

#[test_case(Rule::FStringInI18NFuncCall,Path::new("INT001.py"); "INT001")]
#[test_case(Rule::FormatInI18NFuncCall, Path::new("INT002.py"); "INT002")]
#[test_case(Rule::PrintfInI18NFuncCall, Path::new("INT003.py"); "INT003")]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy());
let diagnostics = test_path(
Path::new("flake8_i18n").join(path).as_path(),
&settings::Settings::for_rule(rule_code),
)?;
assert_yaml_snapshot!(snapshot, diagnostics);
Ok(())
}
}
91 changes: 91 additions & 0 deletions crates/ruff/src/rules/flake8_i18n/rules.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
use rustpython_parser::ast::{Constant, Expr, ExprKind, Operator};

use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::types::Range;

#[violation]
pub struct FStringInI18NFuncCall;

impl Violation for FStringInI18NFuncCall {
#[derive_message_formats]
fn message(&self) -> String {
format!("f-string is resolved before function call; consider `_(\"string %s\") % arg`")
}
}

#[violation]
pub struct FormatInI18NFuncCall;

impl Violation for FormatInI18NFuncCall {
#[derive_message_formats]
fn message(&self) -> String {
format!("`format` method argument is resolved before function call; consider `_(\"string %s\") % arg`")
}
}
#[violation]
pub struct PrintfInI18NFuncCall;

impl Violation for PrintfInI18NFuncCall {
#[derive_message_formats]
fn message(&self) -> String {
format!("printf-style format is resolved before function call; consider `_(\"string %s\") % arg`")
}
}

/// Returns true if the [`Expr`] is an internationalization function call.
pub fn is_i18n_func_call(func: &Expr, functions_names: &[String]) -> bool {
if let ExprKind::Name { id, .. } = &func.node {
functions_names.contains(id)
} else {
false
}
}

/// INT001
pub fn f_string_in_i18n_func_call(args: &[Expr]) -> Option<Diagnostic> {
if let Some(first) = args.first() {
if matches!(first.node, ExprKind::JoinedStr { .. }) {
return Some(Diagnostic::new(
FStringInI18NFuncCall {},
Range::from(first),
));
}
}
None
}

/// INT002
pub fn format_in_i18n_func_call(args: &[Expr]) -> Option<Diagnostic> {
if let Some(first) = args.first() {
if let ExprKind::Call { func, .. } = &first.node {
if let ExprKind::Attribute { attr, .. } = &func.node {
if attr == "format" {
return Some(Diagnostic::new(FormatInI18NFuncCall {}, Range::from(first)));
}
}
}
}
None
}

/// INT003
pub fn printf_in_i18n_func_call(args: &[Expr]) -> Option<Diagnostic> {
if let Some(first) = args.first() {
if let ExprKind::BinOp {
op: Operator::Mod { .. },
left,
..
} = &first.node
{
if let ExprKind::Constant {
value: Constant::Str(_),
..
} = left.node
{
return Some(Diagnostic::new(PrintfInI18NFuncCall {}, Range::from(first)));
}
}
}
None
}
73 changes: 73 additions & 0 deletions crates/ruff/src/rules/flake8_i18n/settings.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};

use ruff_macros::{CacheKey, ConfigurationOptions};

#[derive(
Debug, PartialEq, Eq, Serialize, Deserialize, Default, ConfigurationOptions, JsonSchema,
)]
#[serde(
deny_unknown_fields,
rename_all = "kebab-case",
rename = "Flake8I18NOptions"
)]
pub struct Options {
#[option(
default = r#"["_", "gettext", "ngettext"]"#,
value_type = "list[str]",
example = r#"function-names = ["_", "gettext", "ngettext", "ugettetxt"]"#
)]
/// The function names to consider as internationalization calls.
pub function_names: Option<Vec<String>>,
#[option(
default = r#"[]"#,
value_type = "list[str]",
example = r#"extend-function-names = ["ugettetxt"]"#
)]
#[serde(default)]
/// Additional function names to consider as internationalization calls, in addition to those
/// included in `function-names`.
pub extend_function_names: Vec<String>,
}

#[derive(Debug, CacheKey)]
pub struct Settings {
pub functions_names: Vec<String>,
}

fn default_func_names() -> Vec<String> {
["_", "gettext", "ngettext"]
.iter()
.map(std::string::ToString::to_string)
.collect()
}

impl Default for Settings {
fn default() -> Self {
Self {
functions_names: default_func_names(),
}
}
}

impl From<Options> for Settings {
fn from(options: Options) -> Self {
Self {
functions_names: options
.function_names
.unwrap_or_else(default_func_names)
.into_iter()
.chain(options.extend_function_names)
.collect(),
Copy link
Member

Choose a reason for hiding this comment

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

I changed this to use an iterator.

}
}
}

impl From<Settings> for Options {
fn from(settings: Settings) -> Self {
Self {
function_names: Some(settings.functions_names),
extend_function_names: vec![],
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
---
source: crates/ruff/src/rules/flake8_i18n/mod.rs
expression: diagnostics
---
- kind:
name: FStringInI18NFuncCall
body: "f-string is resolved before function call; consider `_(\"string %s\") % arg`"
suggestion: ~
fixable: false
location:
row: 1
column: 2
end_location:
row: 1
column: 14
fix:
edits: []
parent: ~

Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
---
source: crates/ruff/src/rules/flake8_i18n/mod.rs
expression: diagnostics
---
- kind:
name: FormatInI18NFuncCall
body: "`format` method argument is resolved before function call; consider `_(\"string %s\") % arg`"
suggestion: ~
fixable: false
location:
row: 1
column: 2
end_location:
row: 1
column: 21
fix:
edits: []
parent: ~

Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
---
source: crates/ruff/src/rules/flake8_i18n/mod.rs
expression: diagnostics
---
- kind:
name: PrintfInI18NFuncCall
body: "printf-style format is resolved before function call; consider `_(\"string %s\") % arg`"
suggestion: ~
fixable: false
location:
row: 1
column: 2
end_location:
row: 1
column: 15
fix:
edits: []
parent: ~

1 change: 1 addition & 0 deletions crates/ruff/src/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ pub mod flake8_debugger;
pub mod flake8_django;
pub mod flake8_errmsg;
pub mod flake8_executable;
pub mod flake8_i18n;
pub mod flake8_implicit_str_concat;
pub mod flake8_import_conventions;
pub mod flake8_logging_format;
Expand Down
Loading