Comments (12)
@sstern6, @rrrene: I fully agree that semantics checking should be in a separate or multiple separate rules.
I just don't see how we could add a new rule for the kind of indentation and still keep a user friendly interface. It would be really counter intuitive to have a tabs or spaces rule and a 2 space vs 4 space rule and then possibly additional ones in the future. Written naively like the current tabs vs spaces rule you would end up with something that wants incompatible changes on the same line. To avoid that the new code would have to know about the other rule either directly or indirectly, that seems wrong to me.
What i would like to do is create a new rule that checks for arbitrary indentation kind (default to 2 spaces, but can be overridden to any syntax legal indentation), and deprecate the tabs or spaces rule when it's ready. That way we don't break anyone's current config, while maintaining the current usability.
from credo.
This will be part of v0.4.0
or v0.5.0
. Let's keep this issue open and I will ping you when it's done! 👍
from credo.
Cool. Thanks.
On Wednesday, 9 March 2016, René Föhring [email protected] wrote:
This will be part of v0.4.0 or v0.5.0. Let's keep this issue open and I
will ping you when it's done! [image: 👍]—
Reply to this email directly or view it on GitHub
#51 (comment).
from credo.
I see you've released 0.5 recently. Any update on this? If no one's working on it i could take a look.
from credo.
Please do. 0.5
was an estimation which did not hold up to reality.
This would be a new check in my opinion, which would check all intendation to be multiples of the base (2 for example). It would have to ignore multi-line strings.
Any other thoughts? /cc @sstern6
from credo.
There's already the consistency check "TabsOrSpaces". I think that could be generalized to check for consistent indentation. What do you think? @rrrene
from credo.
The problem with checking for consistent intendation exceeds the responsibility of the TabsOrSpaces
check in my book. You can absolutely try to integrate it into the existing check, but I'd imagine that to be harder than creating another check for intendation using a fix amount of spaces.
Please also note that you will also probably not want to deal to deal with semantically correct indentation:
module_names =
mod_list
|> Credo.Code.Name.full
|> List.wrap
{ast, modules ++ module_names}
where the last two lines are indented inconsistently. Checking this would be a challenging task, I guess.
from credo.
IMO I agree with @rrrene this should be a separate rule, including this into TabsOrSpaces
will make that rule extremely more complicated and fragile adding this new functionality.
In my experience with working on the indent rule in eslint
, dealing with indentation gets increasingly more complicated as you keep growing the rules requirements/functionality(dont let this discourage you from working on it! 😄 ).
That being said, I think if this were to be accepted as a new rule proposal, it should be well scoped to be realistic getting this into the code base with reviews, feedback, etc to be implemented within the next few major releases.
For example, having a rule that is IndentationOfBaseTwo
that only checks indentation of 2 spaces and throws an error for anything more or less. I mean i am open to discussion about why we should have multiple options of indentation but I think 2 is pretty standard, unless im missing something.
Then after this has been flushed out and 1.0 has been released we can increase the flexibility of the rule.
Thoughts @rrrene @Lysholm @henryj ???????
Thanks
from credo.
I agree on everything but this:
That way we don't break anyone's current config, while maintaining the current usability.
We are explicitly losing usability if we require people to define their preference. This very thing is why consistency checks were thought up in the first place.
But I think this can be beneficial as an optional check for people to utilize who want to do so.
from credo.
@rrrene: I was unclear about what i was referring to there, but i take it you disagree with my proposed default? ("2 spaces") And would rather see the default be the most commonly occurring indentation? If that reading is correct then i fully agree, and that's what I've started working on.
from credo.
Hi guys, just stumbled over this discussion. Is this feature still planned? I would really like to add this rule to my projects!
from credo.
@DieMaske we won't implement this in the near future, especially not since elixirfmt
seems to become reality: https://summerofcode.withgoogle.com/projects/#5205518009237504
After re-reading this issue's conversation, I think that checking intendation based on a preference like "2 spaces" is a problem that just seems easy on the surface.
I anyone disagrees with me and is willing to provide a working check that handles edge-cases in a PR then I would merge it, but I am unable to sink time into this myself. And, as I said, I would hesitate to implement something like this before the GSoC has concluded.
from credo.
Related Issues (20)
- Predicate callbacks from behavior are not ignored HOT 2
- Issue with Unused variables confused about the strategy HOT 1
- missing spec on unquoted function raise error HOT 3
- Credo.Check.Warning.WrongTestFileExtension displayed in non-test module in v1.7.4 HOT 8
- NoAmbiguousAliases fails on function param with name alias HOT 3
- "Most of the time you are using the multi-alias/require/import/use syntax..." with compile_env HOT 4
- Gracefully handle output of bitstring `:message` in `%Credo.Issue{}` HOT 8
- False positive: spec in quoted HOT 3
- False Positive: Macro sigils with uppercase words HOT 4
- `UnusedKeywordOperation` check should ignore `Keyword.validate!/2` HOT 4
- Arguable advice from `Credo.Check.Refactor.AppendSingleItem` HOT 1
- Predicate function names should not start with 'is', and should end in a question mark flagged for :ets.whereis HOT 2
- Not ready for Elixir 1.16: a lot of 'warning: negative steps are not supported in Enum.slice/2 HOT 1
- Elixir 1.17 issues HOT 7
- Dependency divergence on compile HOT 2
- Unicode characters (arrows) that indicate priority of found issues not printed correctly HOT 2
- Credo.Check.Warning.Dbg: doesn't warn when using `&dbg/1`
- Error `mix credo` on Elixir 1.17 and Erlang/OTP 27 HOT 4
- Credo.Check.Design.SkipTestWithoutComment and many others, fail to run (Elixir 1.17 issue?) HOT 1
- Error upgrading project to Elixir 1.17 HOT 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from credo.