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.

Sign up for GitHub

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

Jump to bottom

[#6] Check for usage of 'head' for lists #22

Merged
merged 2 commits into from
Apr 12, 2020
Merged

[#6] Check for usage of 'head' for lists #22

merged 2 commits into from
Apr 12, 2020

Conversation

chshersh
Copy link
Contributor

Resolves #6

It worked! On my local test case I saw this output:

Analysis
    { analysisModuleCount = 12
    , analysisLinesOfCode = 0
    , analysisUsedExtensions = 0
    , analysisObservations =
        [ Observation
            { observationId = Id {unId = ""}
            , observationInspectionId = Id {unId = "HEAD"}
            , observationLoc = SrcSpanOneLine "app/Main.hs" 7 7 21
            , observationFile = "app/Main.hs"
            }
        ]
    }

@chshersh chshersh added the essential 🍞 Core fundamental features label Apr 12, 2020
@chshersh chshersh requested a review from vrom911 as a code owner April 12, 2020 14:17
@chshersh chshersh self-assigned this Apr 12, 2020
hint-man[bot]
hint-man bot approved these changes Apr 12, 2020
View reviewed changes
Copy link
Contributor

@hint-man hint-man bot left a comment

Choose a reason for hiding this comment

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

There is no place for me here... I will choose the truth I like.

chshersh
chshersh commented Apr 12, 2020
View reviewed changes
src/Stan/Hie/Debug.hs Show resolved Hide resolved
src/Stan/Observation.hs
@@ -13,7 +13,7 @@ module Stan.Observation
, prettyShowObservation
) where

import GHC (SrcSpan)
import SrcLoc (RealSrcSpan)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to change this since it's how it's stored in HIE files, but fortunately, it still has all necessary information

src/Stan/Observation.hs
@@ -24,7 +24,8 @@ import Stan.Inspection (Inspection)
data Observation = Observation
{ observationId :: !(Id Observation)
, observationInspectionId :: !(Id Inspection)
, observationLoc :: !SrcSpan
, observationLoc :: !RealSrcSpan
, observationFile :: !FilePath
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went for the simplest solution for now and just added FilePath as a field. We can change this later...

vrom911
vrom911 reviewed Apr 12, 2020
View reviewed changes
Copy link
Member

@vrom911 vrom911 left a comment

Choose a reason for hiding this comment

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

Amazing job! I am so excited about the very first working inspection 😻

src/Stan/Analysis.hs
@@ -23,7 +25,7 @@ data Analysis = Analysis
{ analysisModuleCount :: !Int
, analysisLinesOfCode :: !Int
, analysisUsedExtensions :: !Int
, analysisObservations :: !() -- TODO: use Observation type later
, analysisObservations :: ![Observation]
Copy link
Member

Choose a reason for hiding this comment

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

I can imagine that in future we would like to either keep a sized list here instead or to keep a number of observations along the way. That would be handy for report + filtering/ searching. What do you think? Can keep this in mind :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vrom911 Nice use case for the sized list! I imagine we will find more places in our codebase where we can use sized list. This data type probably will change in the future, but I agree, let's keep it in mind 🙂

src/Stan/Analysis/Partial.hs

guard $ and
[ occName == "head"
, modul == "GHC.List"
Copy link
Member

Choose a reason for hiding this comment

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

base has several modules that export head. Are you sure that we don't need to look for each of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vrom911 Yes, I'm sure. HIE files record the original place of where the function is implemented initially.

Copy link
Member

Choose a reason for hiding this comment

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

Good to know! Thanks for the explanation, @chshersh ❤️

src/Stan/Analysis/Partial.hs Outdated

mkPartialObservation :: RealSrcSpan -> Observation
mkPartialObservation srcSpan = Observation
{ observationId = Id "" -- TODO: how to create observation id?
Copy link
Member

Choose a reason for hiding this comment

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

That is a good question. I would like to have a number of observation plus type in the name, I think 🤔 Or maybe Module name plus something?

1-Module-HEAD
2-HEAD
3-Warning-HEAD

any of them look normal to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like 2-HEAD 🙂 Can implement this 👍

src/Stan/Analysis/Partial.hs Outdated
mkPartialObservation :: RealSrcSpan -> Observation
mkPartialObservation srcSpan = Observation
{ observationId = Id "" -- TODO: how to create observation id?
, observationInspectionId = Id "HEAD"
Copy link
Member

Choose a reason for hiding this comment

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

I think, that having several smart constructors with the specified inspection links would make sense here.
Anyway, we have to know all of the Inspections beforehand.

src/Stan/Inspection.hs Outdated
, "* Use explicit pattern-matching over lists"
]
, inspectionCategory =
one $ Category "Partial" -- TODO: should we convert this to values to avoid typos?
Copy link
Member

Choose a reason for hiding this comment

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

Yes! That would be nice, and also handy!

Copy link
Member

Choose a reason for hiding this comment

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

We also will need a list of all categories

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vrom911 Should we move Category in a separate module then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created an issue to do this separately: #25

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good 👍

src/Stan/Inspection.hs Outdated
inspections :: [Inspection]
inspections =
[ Inspection
{ inspectionId = Id "HEAD" -- TODO: we need to come up with naming scheme for inspection ids
Copy link
Member

Choose a reason for hiding this comment

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

Yes, another good question..
Should we use some coding scheme, like STAN-0001-HEAD, so we may have some other inspections in the future (like GHC-Warning-0001)? I am really not sure if this is helpful 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the STAN-0001-HEAD scheme 😍 I agree with your reasoning that we probably will have other types of warnings, so adding STAN as a prefix is a good idea 👍

src/Stan/Inspection.hs Outdated
inspections =
[ Inspection
{ inspectionId = Id "HEAD" -- TODO: we need to come up with naming scheme for inspection ids
, inspectionName = "Partial 'head'" -- TODO: does it make sense?
Copy link
Member

Choose a reason for hiding this comment

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

Maybe with semicolon Partial: head? And should we add base/head?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent idea 👌

This was referenced Apr 12, 2020
Closed
Closed
@chshersh chshersh force-pushed the chshersh/6-Check-for-usage-of-head branch from a40b3e9 to f30493a Compare April 12, 2020 17:28
vrom911
vrom911 approved these changes Apr 12, 2020
View reviewed changes
Copy link
Member

@vrom911 vrom911 left a comment

Choose a reason for hiding this comment

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

Cool 🚀

@vrom911 vrom911 merged commit ebebd66 into master Apr 12, 2020
@vrom911 vrom911 deleted the chshersh/6-Check-for-usage-of-head branch April 12, 2020 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vrom911 vrom911 vrom911 approved these changes

@hint-man hint-man hint-man approved these changes

Assignees

@chshersh chshersh

Labels
essential 🍞 Core fundamental features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Check for usage of 'head' for lists
2 participants
@chshersh @vrom911