8000 Drop pathtype, part one by robrix · Pull Request #687 · github/semantic · GitHub
[go: up one dir, main page]

Skip to content
This repository was archived by the owner on Apr 1, 2025. It is now read-only.

Drop pathtype, part one #687

Merged
merged 7 commits into from
Jul 18, 2022
Merged

Drop pathtype, part one #687

merged 7 commits into from
Jul 18, 2022

Conversation

robrix
Copy link
Contributor
@robrix robrix commented Jul 14, 2022

This PR drops semantic-source's dependency on pathtype, selecting languages from FilePaths instead. I'll release the new version once this PR is approved.

Our other packages get semantic-source from cabal, and are constrained to exclude 0.2+, so they shouldn't be disturbed by this change. Updating them to drop the pathtype dependency is postponed to a later PR.

@robrix robrix requested a review from a team as a code owner July 14, 2022 14:15
Copy link
Contributor Author
@robrix robrix left a comment

Choose a reason for hiding this comment

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

Ready for review.

cabal v2-run --project-file=cabal.project.ci semantic-source:test
cd semantic-source && cabal v2-run --project-file=cabal.project.ci semantic-source:test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're excluding this from the top-level project now, so we cd in first.

@@ -15,7 +15,6 @@ packages: semantic
semantic-ruby
semantic-rust
semantic-scope-graph
semantic-source
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, and in cabal.project.ci, we remove semantic-source from the top-level project, ensuring that it'll be fetched from hackage. This makes it possible to factor out PRs like this one, at the cost of making it less convenient to change things across both semantic (or any other local package) and semantic-source.

For this PR I think that's worth it; for the next one I might end up reinstating this. But I wanted to factor the PRs out to begin with to limit scope and to be sure we're handling versioning appropriately.

Copy link
Contributor

Choose a reason for hiding this comment

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

at the cost of making it less convenient to change things across both

To clarify, this means that the process would be to merge the PR that touches the "downstream" package, cut a new release and deploy it to hackage, and then open the PR in the "upstream" package?

(Not meaning to suggest a different approach, just making sure I understand how the process would be impacted.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. Or more precisely: reintroduce semantic-source as a local package (i.e. add it back to this list) without committing that change, and then do the hackage dance after the fact.

I think the real moral here is that we probably released semantic-source to hackage too early. Lesson learned.

Comment on lines -99 to +103
forPath :: Path.PartClass.AbsRel ar => Path.File ar -> Language
forPath :: FilePath -> Language
forPath path =
let spurious lang = lang `elem` [ "Hack" -- .php files
, "GCC Machine Description" -- .md files
, "XML" -- .tsx files
]
allResults = Lingo.languageName <$> Lingo.languagesForPath (Path.toString path)
allResults = Lingo.languageName <$> Lingo.languagesForPath path
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only real code change.

Copy link
Contributor

Choose a reason for hiding this comment

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

🥲

@robrix robrix mentioned this pull request Jul 15, 2022
@@ -15,7 +15,6 @@ packages: semantic
semantic-ruby
semantic-rust
semantic-scope-graph
semantic-source
Copy link
Contributor

Choose a reason for hiding this comment

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

at the cost of making it less convenient to change things across both

To clarify, this means that the process would be to merge the PR that touches the "downstream" package, cut a new release and deploy it to hackage, and then open the PR in the "upstream" package?

(Not meaning to suggest a different approach, just making sure I understand how the process would be impacted.)

Comment on lines -99 to +103
forPath :: Path.PartClass.AbsRel ar => Path.File ar -> Language
forPath :: FilePath -> Language
forPath path =
let spurious lang = lang `elem` [ "Hack" -- .php files
, "GCC Machine Description" -- .md files
, "XML" -- .tsx files
]
allResults = Lingo.languageName <$> Lingo.languagesForPath (Path.toString path)
allResults = Lingo.languageName <$> Lingo.languagesForPath path
Copy link
Contributor

Choose a reason for hiding this comment

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

🥲

@robrix robrix merged commit 48ca560 into main Jul 18, 2022
@robrix robrix deleted the pathological,-part-one branch July 18, 2022 16:27
@robrix
Copy link
Contributor Author
robrix commented Jul 18, 2022

Released.

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

Successfully merging this pull request may close these issues.

2 participants
0