New Check: OpenBrowser

[kerrd89]

• add open_browser.ex as a warning with associated tests
• default params only have this check running on test files

[rrrene]

Thx for trying to put something together 👍

I have to ask though: was this PR actually meant for this repo? (honest question, mistakes happen 😅)

[kerrd89]

Thx for trying to put something together 👍

I have to ask though: was this PR actually meant for this repo? (honest question, mistakes happen 😅)

A credo check for open_browser being left in tests? Yes, it has been valuable to our organization and it was suggested I push this change to the larger elixir ecosystem. With LiveView and associated tests becoming prevalent it seemed valuable.

Happy to take any feedback as to why this "try" is not worthy. Or close this PR if not.

[nathanl]

@rrrene I suggested making a PR here. open_browser is a bit like IO.inspect or dbg when working on LiveView tests.

[rrrene]

Happy to take any feedback as to why this "try" is not worthy. Or close this PR if not.

I am not a native speaker, but "worthy" seems like a pretty loaded term to me. No need to get this defensive this fast.

As to why I thought this might not be meant for this project: Most PRs, esp. for new checks, include a reasoning for the check's existence and some rationale why it was structured the way it is.

This PR seemed more like a PR between colleagues or co-workers (in all seriousness: I don't want to sound like an angry old man yelling "get off my lawn!", but in the PRs I created to OSS projects I never once submitted a two item bullet point list for a description).

@rrrene I suggested making a PR here. open_browser is a bit like IO.inspect or dbg when working on LiveView tests.

Fair point. However, Credo does not yet have framework specific checks up until now. I am wondering if it would be worth the effort to create a generalized check for this, which users can configure to their needs. @kerrd89 WDYT?

[kerrd89]

Closing and moving on sounds like it is not within the scope of credo today. Thanks for the conversation.

[rrrene]

Again, not a native speaker. Maybe something was "lost in translation" here:

I was suggesting that we could rework this into a more generalized solution and trying to ask you if you were up for it.

Sorry if my words did not convey that intention 🙁

[paulo-ferraz-oliveira]

@rrrene, does Credo support "a list of invalid functions" as a check? (I don't remember by heart) That could be a more generic solution where maybe a "curated" list could make its way into it. Elvis (Erlang-land) does that via a no_call_functions rule, that's internally also used to implement two other rules: no_common_caveats_call and no_debug_call for stuff which is "common" to find in the wild. But I understand the "framework-specific" argument, since it's a fine line to walk. In this case what would happen if I were to implement a function called open_browser, in tests, which was not the intended LiveView target?

[rrrene]

@rrrene, does Credo support "a list of invalid functions" as a check? (I don't remember by heart)

@paulo-ferraz-oliveira That's what I was trying to suggest: We already have ForbiddenModule and it would be nice to have that for functions as well.

I was trying to wrap my head around how complex it would be to have something that doesn't produce a lot of false positives and is highly customizable in light-weight fashion.

[kerrd89]

It seems to me like there is a need for credo-like functionality targeted to LiveView implementations. Perhaps a new module or an extension ofcredo in some way.

For example, eslint has plugins for react.

While the generalized forbidden function calls may be helpful, its a customizable tool rather than something plug and play for known use cases.

[kerrd89]

I also suggest updating the README to include instructions that new rules should go through the proposal process, which appears to have been a flaw in my process.