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

Fixed order of FilePosition components #3435

Merged
merged 3 commits into from
Aug 27, 2024

Conversation

mahrud
Copy link
Member

@mahrud mahrud commented Aug 26, 2024

  • moved a dummy declaration to expr.d
  • fixed the order of FilePosition components

c.f. #3431 (comment)

@mahrud mahrud requested a review from pzinn August 26, 2024 01:48
M2/Macaulay2/d/stdiop.d Outdated Show resolved Hide resolved
@mahrud
Copy link
Member Author

mahrud commented Aug 26, 2024

To be clear, this PR doesn't change any top-level behavior whatsoever. You can verify with the tests I added, and if you find anything that is changed we should add them as tests to make sure nothing changes by accident.

@pzinn
Copy link
Contributor

pzinn commented Aug 26, 2024

yes I will provide more tests, I have a list of them, I just forgot to include them in my PR

Copy link
Contributor

Choose a reason for hiding this comment

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

thinking about tests -- by defining both f and F, are you suggesting they should have different locations? I think I vaguely considered that at some point (maybe F should only have the right of ->?) but rejected it, don't remember why though.

Copy link
Contributor

@pzinn pzinn Aug 26, 2024

Choose a reason for hiding this comment

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

the simplest one that has all locations distinct is probably

f = () -> 1 + 2
toSequence locate((pseudocode f)_4)

Copy link
Contributor

Choose a reason for hiding this comment

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

most of my "tests" actually involve errors -- not sure how to turn them into assert type tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

are you suggesting they should have different locations

I can see it both ways, but I'm just adding the test lines so if they change unintentionally we find out.

Copy link
Member Author

Choose a reason for hiding this comment

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

most of my "tests" actually involve errors -- not sure how to turn them into assert type tests.

You can use capture. There are several examples of this already.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I think this can wait till my next PR on this kind of stuff.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you already have tests that you know what to expect and are correct now, just paste them here or send them to me and I can put them in.

@mahrud mahrud merged commit 3eb3722 into Macaulay2:development Aug 27, 2024
5 checks passed
@mahrud mahrud deleted the quickfix/locate branch August 27, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants