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

SetLoadFields #226

Merged
merged 5 commits into from
Sep 28, 2023
Merged

SetLoadFields #226

merged 5 commits into from
Sep 28, 2023

Conversation

Busschers
Copy link
Contributor

This is my first pull request for the AL Guidelines so I hope it is a bit like it is meant to be.

I couldn't find a best practice for SetLoadFields so I decided to create one on my own.

In the second part of the good/bad code I've described that you should record the SetLoadFields before the lines for filtering, technical whise there is no need to do this but I think it is better to have an guideline where the SetLoadFields should be placed so if there is a reason why it should be placed after the filters then we should change that.

@Busschers
Copy link
Contributor Author

Busschers commented Jun 6, 2023 via email

@Busschers
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Dysel BV"

@Busschers
Copy link
Contributor Author

@microsoft-github-policy-service agree

@waldo1001
Copy link
Collaborator

Totally agree with the first one - I would even add to ALWAYS add a setloadfields, and, if necessary, make it extensible.

Second one - I don't know. I personally want it just before Get (or find..) so it's very clear what I'm getting .. 🤔

Copy link
Contributor

@PeterConijn PeterConijn left a comment

Choose a reason for hiding this comment

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

Looks good! I say: get it in there

@JeremyVyska, @ajkauffmann? This has been open for a while. Can you guys review so we can get it moving?

@JeremyVyska JeremyVyska merged commit 9812ecc into microsoft:main Sep 28, 2023
2 checks passed
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.

5 participants