WIP - Bug 3038 editor search add text search button#3100
Conversation
|
Hi Rutrajv! Thanks for helping out! Awesome work! I ran the project locally with your changes. While I get 3 of the errors that you are showing here, I didn't get any of the import errors. Is there anything else I need to know about your setup? Regarding the other errors: Flow types can be a bit tricky if you are just starting out with them. If you aren't familiar with typing, here is the quick version: typing arguments and outputs allows us to know that the inputs and outputs of a given function are correct. This simplifies testing and gives us more certainty in our code. Lets start with a simple error and trace our way back. This is telling us that import type { FileSearchModifiers, SymbolSearchType } from "../../reducers/ui";What we can do now is navigate into that file (src/reducers/ui.js) and take a look. There we can find a declaration for export type SymbolSearchType = "functions" | "variables";This flow type is pretty limited. The only acceptable strings for a SymbolSearchType are "functions", and "variables" ( export type SymbolSearchType = "functions" | "variables" | "text";The same can go for where the default value for ToggleSymbolSearchOpts is an empty object, but both the Hopefully this helps! let me know if you have more questions! |
codehag
left a comment
There was a problem hiding this comment.
Looks good so far! A few comments. Since im not too familiar with the issue, but given that we are checking if symbolSearch is disabled for the text search, I think it might make sense to split the logic for the text search and the symbol search.
| function searchTypeBtn(searchType) { | ||
| let active = | ||
| (symbolSearchOn && | ||
| selectedSymbolType == searchType && |
There was a problem hiding this comment.
we should prefer identity equality === instead of equality == here!
| const { symbolSearchOn, selectedSymbolType } = this.props; | ||
|
|
||
| function searchTypeBtn(searchType) { | ||
| let active = |
There was a problem hiding this comment.
Since this isn't redefined, we should prefer const over let
| (symbolSearchOn && | ||
| selectedSymbolType == searchType && | ||
| searchType !== "text") || | ||
| (!symbolSearchOn && searchType === "text"); |
There was a problem hiding this comment.
This is a pretty complex boolean. I think it would be more readable if we split this into functions. Maybe something like this:
const { symbolSearchOn, selectedSymbolType } = this.props;
function isSymbolSearchOn(searchType) {
// since the boolean is so complex, consisting of three parts, lets break it up
// and use clear variable names to make it obvious what is happening
const isSymbolSearch = selectedSymbolType === searchType;
const notSearchText = searchType !== "text";
return (symbolSearchOn && isSearchType && notSearchText);
}
function isSearchText(searchType) {
return (!symbolSearchOn && searchType === "text");
}
function isTextSearch(searchType) {
const { symbolSearchOn } = this.props;
return (symbolSearchOn && searchType === "text");
}
const active = isSymbolSearchOn() || isSearchText()
But, this still looks rather complex to me. So I suspect something might need to change about our approach here ...
This is a "if you have time" suggestion, as we can definitely do the simpler version.
From reading the code, we can see that SymbolSearch and SearchText are two different things that are at odds with eachother. You mentioned elsewhere that you have problems with the flow typing. SymbolSearchType doesn't have a "text" as an available input.
Since you are explicitly checking if SymbolSearchOn is off when it is of type text -- we probably do not want to change that flow type to accept that as a valid type. It doesnt make sense for "text" to be a valid SymbolSearchType if symbolSearchOn is false.
Maybe we need a new function (it might even share a lot of the code), which is "searchTextBtn" with a new variable that has its own flow type? You can copy the SymbolSearchType and make a TextSearchType with just "text" as a valid value. That might simplify the code!
This solution will take longer though, and will require some renaming. Let me know what you think and if you need help.
|
Hey @codehag Thanks a lot for your help, yes I'm new to flow. Regarding the simplification of Let me work on your suggestions. |
Codecov Report
@@ Coverage Diff @@
## master #3100 +/- ##
==========================================
+ Coverage 47.43% 47.54% +0.11%
==========================================
Files 97 97
Lines 4018 4036 +18
Branches 826 828 +2
==========================================
+ Hits 1906 1919 +13
- Misses 2112 2117 +5
Continue to review full report at Codecov.
|
|
Hey @codehag I've fixed some of the errors by not checking for The Hence the |
|
Hi Ruturajv! Nice work, its starting to look cleaner. I think that your impulse with checking for "text" as a type was a good one actually. Also, I agree that searchBtn was originally intended for SymbolSearches. I think the changes you made are good in that they are cleaner, but they also hide that there is a third potential value for searchType, which is "text". It would be better to introduce a new flow type, rather than removing an existing one. How about this as a solution: Lets expand the definition of searchType like so: // lines 81-84
type TextSearchType = "text";
type ToggleSymbolSearchOpts = {
toggle: boolean,
searchType: SymbolSearchType | TextSearchType
};then we can do the check for Further, with your case statement, we can do the same, adding a case for "text". |
|
|
||
| // Check if searchType is "text" | ||
| if (!["functions", "variables"].includes(searchType)) { | ||
| this.props.toggleSymbolSearch(false); |
There was a problem hiding this comment.
hmm, this seems a bit out of date given what we're doing now. Maybe we want it to be: setEditorSearchType and that can include text
|
@jasonLaster hmm.. yaa seems lot might change here... even after @codehag suggestions, these errors persist |
b72889a to
0e41604
Compare
0e41604 to
6433397
Compare
|
Hi @ruturajv! Thanks again for doing this. I went in and I wrestled with the code a bit, since upon working with it I realized it needed a refactor. Here is what I did:
If anything is confusing, ping me! I am more than happy to explain further :) |
|
@codehag Thanks for the huge rewrite :) Let me pull and go through it. |
codehag
left a comment
There was a problem hiding this comment.
made some comments about stuff i missed too, ill probably get to them later today
|
|
||
| function searchTypeBtn(searchType) { | ||
| const active = isButtonActive(searchType); | ||
| const toggle = selectedSymbolType == searchType; |
There was a problem hiding this comment.
this should be identity equality ===
| const getExpressionSel = createSelector(expressionsSel, expressions => input => | ||
| expressions.find(exp => exp.input == input) | ||
| ); | ||
| expressions.find(exp => exp.input == input)); |
There was a problem hiding this comment.
should be identity equality ===
| let bar = 2; | ||
| const baz = 3; | ||
| const a = 4, | ||
| b = 5; |
There was a problem hiding this comment.
style nit: this should be
const a = ...;
const b = ...;
There was a problem hiding this comment.
this actually should be this way becase we're testing the weird behavior :) We want to make sure we find those vars too :P
@ruturajv did this i believe
…b.com:ruturajv/debugger.html into bug-3038-editor-search-add-text-search-button
Associated Issue: #3038
Even though things are working... the flow gives following errors. Except for the SearchBar.js I'm not sure. Could you guys help me with this?