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

Refactoring across the repo #33

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

SpotlightForBugs
Copy link
Contributor

@SpotlightForBugs SpotlightForBugs commented Oct 8, 2023

Description

used Deepsource Autofix

Type of change

Refactored two things:

  • refactor: convert logical operator to optional chaining

The optional chaining operator can be used to perform null checks before accessing a property, or calling a function.

  • refactor: remove unnecessary boolean casts

In contexts such as an if statement's test where the result of the expression will already be coerced to a Boolean, casting to a Boolean via double negation (!!) or a Boolean call is unnecessary.

My Bootstrap Academy username: SFB

deepsource-io bot and others added 12 commits October 8, 2023 15:13
The [optional chaining](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining) operator can be used to perform null checks before accessing a property, or calling a function.
refactor: convert logical operator to optional chainining
In contexts such as an `if` statement's test where the result of the expression will already be coerced to a `Boolean`, casting to a `Boolean` via double negation (`!!`) or a `Boolean` call is unnecessary.
Copy link
Contributor Author

@SpotlightForBugs SpotlightForBugs left a comment

Choose a reason for hiding this comment

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

I think that these changes help more people to start contributing because they don’t have to deal with !!! or !! anymore

@SpotlightForBugs
Copy link
Contributor Author

SpotlightForBugs commented Oct 10, 2023

@Defelo was denkst du? (Kein Bock immer Konflikte zu beheben)


@Defelo what do you think? (I don’t feel like always resolving conflicts)

@Defelo Defelo self-requested a review October 11, 2023 16:10
@Defelo Defelo self-assigned this Oct 11, 2023
Copy link
Member

@Defelo Defelo left a comment

Choose a reason for hiding this comment

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

There are lots of unneeded parentheses now, mostly where double negation was removed, this should be fixed before merging.

@@ -244,7 +244,7 @@ export default defineComponent({
}
}

if (!!!date.value) isValid = false;
if (!date.value) isValid = false;
if (!!!time.value || time.value == '00:00:00') isValid = false;
Copy link
Member

Choose a reason for hiding this comment

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

What about this one?

Comment on lines -35 to +39
function setExpand(index) {
for (let i = 0; i < faqs.value.length; i++) {
// invert expand-state for clicked item, close all others
faqs.value[i].expand = i == index ? !faqs.value[i].expand : false;
function setExpand(index, value) {
if (value) {
for (let i = 0; i < faqs.value.length; i++) {
faqs.value[i].expand = i == index;
}
Copy link
Member

Choose a reason for hiding this comment

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

This was fixed in a previous pr, why did you change this back?

@@ -45,7 +45,7 @@ export default defineComponent({
"chip-color-12",
];

return !!props.color
return (props.color)
Copy link
Member

Choose a reason for hiding this comment

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

  1. These parentheses are useless.
  2. I think, you can use the ?? operator here.

@Defelo Defelo assigned SpotlightForBugs and unassigned Defelo Oct 11, 2023
@github-actions
Copy link

Preview deployed to https://e3a11daa.academy-preview.pages.dev (total size: 11M)

@Defelo
Copy link
Member

Defelo commented Oct 30, 2023

Hi @SpotlightForBugs, are you still working on this?

@SpotlightForBugs
Copy link
Contributor Author

@Defelo yes, I am

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants