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

Configurable mixed tabs and spaces #848

Open
adrszad opened this issue May 17, 2023 · 4 comments
Open

Configurable mixed tabs and spaces #848

adrszad opened this issue May 17, 2023 · 4 comments

Comments

@adrszad
Copy link

adrszad commented May 17, 2023

I observe this rule is often confusing for the users, especially as no line number is given. It could be a project setting if we use spaces or tabs, and then each occurance of wrong separator could be logged separately with line number.

@mnojek
Copy link
Member

mnojek commented May 31, 2023

Hey @adrszad, thanks for creating the issue and sorry for late reply.

I think it's a valid point that it may be configurable. Would you see it as a rework of the rule W1006 mixed-tabs-and-spaces in a form of a new rule like inconsistent-separators (or some different name) that can take a parameter like separator with possible values tab, space, or mixed and then report only when the rule is violated?

I recall we have discussed it before with @bhirsz and we are aware that opinions are divided whether it's better to use spaces or tabs. That's the reason we decided that maybe it's better to not enforce one or the other, and only detect the case when both of them are mixed within one file.

Personally, I would like Robocop to not enforce tabs or spaces and let people decide what they want to use, and not report the issue until it is explicitly set by the user that one or the other is required. So it'd be kind of a disabled rule until it's explicitly used or configured.

What do you think?

As a sidenote, it may be a good candidate for the community rule (which we plan to add in the future), but I think it may also be included as a built-in rule, because I see it as a beneficial.

@mnojek
Copy link
Member

mnojek commented Jun 15, 2023

@bhirsz @adrszad Any thoughts here? I don't have a strong opinion, so it would be great to discuss what to do with it before the next steps. Feel free to respond whenever you have time. Let's talk 🙂

@bhirsz
Copy link
Member

bhirsz commented Jun 15, 2023

I would like Robocop to not enforce tabs or spaces and let people decide what they want to use

It's actually working like this right now. If user only uses spaces, or tabs, mixed-tabs-and-spaces will not be reported. I'm fine with configuring it so it's possible to use fixed value (report every occurence of tab or space even if it's consistent). What we're missing is reporting on every occurence - which can over clutter log bug is really helpful in catching small typos in larger files.

So it could:

mixed: by default, only check if there are mixed separators
space or tab: validate that only space/tab is used within file

@mnojek
Copy link
Member

mnojek commented Jun 15, 2023

@bhirsz Yes, I know it works like this, but user has no control over what is used, because the rule only reports inconsistency. But it would be good to give the user a way to enforce tabs or spaces for each file.

The only concern I have is how should we introduce it into Robocop, since we already have a rule W1006 mixed-tabs-and-spaces. Having it configurable with new parameter separator and values mixed, tab, space makes the name of the rule no longer applicable. That's why I think we should add a new rule called inconsistent-separators.

Though, there is another issue now - if we want this rule to report on every place where the other separator is used, for the separator=mixed option, it won't be possible to indicate if it should report on all spaces or all tabs. For tab or space options it's trivial, because it would report on the opposite value.

So maybe we should keep also the old rule that will check if the separators are inconsistent in the file and report only once? This way we will have one rule to report if they are mixed, and one to report the exact place where the unwanted separator is used.

There is also a variant to add one more param to the new rule (or even old rule) and allow the users to decide whether it should report once or each occurrence, but I'm not a fan of this solution, since this affects too much how one rule may work.

What do you think?

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

No branches or pull requests

3 participants